feat: only add response header for sampled traces #56
Conversation
costela
requested review from
riandyrn,
ilhamsyahids and
ProtozoaJr
as code owners
|
Hi, @costela First of all, thanks for your PR! However, I'm still not sure if removing the trace ID from the response header is a good idea for the non-sampled trace. The reason is that the trace ID might be used by underlying middleware to do something else, such as check whether the trace context is propagated to the service (because when the trace is not sampled, the context is still propagated to the child service) and maybe for any other use cases. However, I agree with you that we should let the user know when the trace is not sampled because otherwise, the user might think that the trace is sampled when, in reality, it is not. So, what do you think if we just add a new response header called cc: @ilhamsyahids, @ProtozoaJr |
costela
force-pushed
the
only-add-response-header-if-trace-was-sampled
branch
from
b65553e to
17fe0df
Compare
|
We have a similar use case, where a middleware does some reporting on traces, and the "unused" Your suggestion seems totally sufficient for our case as well. However, it doesn't quite fit with the current header customization in PTAL 🙏 |
|
did anyone get a chance to look at the proposed changes? 😇 |
|
Hi, @costela Thanks for the update Can you please also add test for sampled trace? You might need to check the example |
| @@ -32,7 +36,7 @@ func (o optionFunc) apply(c *config) { | |||
| o(c) | |||
| } | |||
|
|
|||
| // Filter is a predicate used to determine whether a given http.request should | |||
| // Filter is a predicate used to determine whether a given [http.Request] should | |||
There was a problem hiding this comment.
I don't think this change is necessary.
There was a problem hiding this comment.
These are using the relatively new doc linking format and make navigating from comments a bit more comfortable.
But of course: if you'd prefer to keep this MR focused, I can just remove the changes.
| @@ -138,7 +142,7 @@ func WithPublicEndpoint() Option { | |||
| // incoming span context. Otherwise, the generated span will be set as the | |||
| // child span of the incoming span context. | |||
| // | |||
| // Essentially it has the same functionality as WithPublicEndpoint but with | |||
| // Essentially it has the same functionality as [WithPublicEndpoint] but with | |||
There was a problem hiding this comment.
I don't think this change is necessary.
middleware.go
Outdated
| @@ -23,7 +24,9 @@ const ( | |||
| // requests. The serverName parameter should describe the name of the | |||
| // (virtual) server handling the request. | |||
| func Middleware(serverName string, opts ...Option) func(next http.Handler) http.Handler { | |||
| cfg := config{} | |||
| cfg := config{ | |||
| TraceSampledResponseKey: defaultTraceSampledResponseHeaderKey, | |||
There was a problem hiding this comment.
I think it's better to set this in here.
|
deprecated by #57 |
|
superceded by #59 |
This changes the behavior of
WithTraceIDResponseHeaderto only add the header if the trace was actually sampled. A trace-id for a trace that wasn't sent anywhere doesn't seem particularly useful?This could be considered a breaking change. Alternatively, we could add an extra option to control that, but that seems a bit overkill IMHO?