http.Request influenced sampling

[toffaletti]

Is your feature request related to a problem? Please describe.
I would like to always sample requests with certain properties. For example, when the request has ?trace=1 and the RemoteAddr is on an internal network the trace will be sampled.

Describe the solution you'd like

type ochttp.Handler struct {
  ....
  StartOptions func(*http.Request) trace.StartOptions
}

Describe alternatives you've considered

1. Injecting a sampler in the http.Handler that ochttp.Handler wraps, using trace.StartSpan(..., trace.Sampler(...)). The problem with this is that the root span added by ochttp might not be sampled.

2. Wrapping ochttp.Handler itself and influencing the ochttp.Handler.StartOptions.Sample somehow. The problem with this is that there isn't really any good way to get context into a trace.Sampler except by closing over some state and we can't modify the closure for every request.

3. Wrapping ochttp.Handler and create an additional outer span where we can set a trace.Sampler via StartSpan TraceOptions that closes over the *http.Request. Adding a meaningless span just for this seems not ideal.

4. Duplicating all of the functionality of ochttp.Handler in another package just to add this. Essentially maintaining a fork.

5. Creating another http.Server on a different port with all of the same endpoint handlers except with a AlwaysSample sampler set. Opening another port isn't an option for us.

[semistrict]

This seems like a good approach to me.

/cc @rakyll

[rakyll]

Cases like this makes me wonder whether we should have provided a hook to customize the created span.

type ochttp.Handler struct {
  ....
  StartSpan func(*http.Request) *trace.Span
}

Then, FormatSpanName or the blacklisting/filtering capabilities we have been discussing can be implemented by the user with full flexibility.

[semistrict]

@rakyll could you elaborate on the reasoning behind returning trace.Span rather than trace.StartOptions as @toffaletti proposed? I think either would work: trace.Span is more flexible (allows adding attributes for example) but trace.StartOptions seems simpler to implement.

[rakyll]

There is a repeating pattern I see when it comes to the need of customizing request spans. We might keep adding different fields or should provide a way for the user to override the implementation entirely.

[toffaletti]

I prefer @rakyll 's more general solution of hooking in at StartSpan if possible, but I'm not sure how that would work since all of the fields of trace.Span are internal and Span being a read-only type seems like the right design.

[semistrict]

@toffaletti I am not following. @rakyll suggested to return a Span from the hook installed in the Handler struct. I don't see anywhere a suggestion to hook into StartSpan.

After thinking about it, I think your original proposal to return a trace.StartOptions is preferable. If you need to add attributes or do something that requires the trace.Span, you can always obtain it from the request context within your handler and call SetAttribute, Annotate, etc. Creating a new Span correctly is pretty complicated (you need to use StartSpanWithRemoteParent in this case) and I think we should spare users from having to deal with this if possible.

[rakyll]

Creating a new Span correctly is pretty complicated (you need to use StartSpanWithRemoteParent in this case) and I think we should spare users from having to deal with this if possible.

Yeah, this is something I agree with. Please ignore the StartSpan suggestion.

[derekperkins]

@toffaletti Are you working on adding this?

[toffaletti]

No, I am still waiting on approval to be able contribute.

[rakyll]

If @Ramonza has nothing against, the following looks good to me.

type ochttp.Handler struct {
  ....
  StartOptions func(*http.Request) trace.StartOptions
}

[semistrict]

Looks good but the field will need to be called something else as there is already a StartOptions field on that struct.

[rakyll]

What about GetStartOptions?

This is the naming pattern used in standard library. For example, net/http.Request has a GetBody where you can optionally set instead of Body.

[rakyll]

Is anyone working on this? If not, can I?