Add baggage to request context #131
Conversation
Adding the baggage to the request context makes it easier for users to pass the baggage along in subsequent requests.
| func setBaggageContextFromHeader(r *http.Request) (*http.Request, bool) { | ||
| baggageHeader := r.Header.Get("Baggage") | ||
| if baggageHeader == "" { | ||
| return r, false | ||
| } | ||
|
|
||
| b, err := baggage.Parse(baggageHeader) | ||
| if err != nil { | ||
| return r, false | ||
| } | ||
|
|
||
| ctx := baggage.ContextWithBaggage(r.Context(), b) | ||
| return r.WithContext(ctx), true | ||
| } | ||
|
|
There was a problem hiding this comment.
Just a side note:
I’m wondering if we need to parse the baggage here, especially to propagate it further. It seems like too much for a thin middleware that is only responsible for k6 integration.
I believe the OTel baggage should already be in the context. In fact, we expect the app to be fully instrumented, with baggage handled across all services in the call chain (i.e., the HTTP/gRPC client must propagate it)
There was a problem hiding this comment.
I agree that parsing the baggage in this middleware might be doing too much.
However, I assume parsing does provide a safeguard by validating and normalizing the baggage(?). What if we:
- Keep the current implementation
- Add a comment explaining this reasoning and the assumption of parsed baggage
- Offer a separate, optional middleware for baggage parsing
There was a problem hiding this comment.
This method is largely a convenience feature for users so they can get set up with the k6 integration more quickly. I'm not opposed to removing this and requiring users to do this plumbing on their own, but I did think it would make it easier to adopt initially.
Keep in mind this parsing doesn't happen every request. It will only occur when there
- the baggage has not already been parsed and placed in the context and
- there is a
Bagggageheader
If those two conditions are true, then we parse the baggage. Again, happy to pull this out if the consensus is this shouldn't exist in our library code.
There was a problem hiding this comment.
the baggage has not already been parsed and placed in the context
From the implementation, it appears that whenever there's a Baggage header present, the Parse function is always called, regardless of whether the baggage has been parsed before (maybe I'm missing something).
I also reviewed the OTel baggage Parse function implementation, it appears there's no internal mechanism to avoid re-parsing.
There was a problem hiding this comment.
I believe that this is not an issue, and it should not block you. Just shared my thoughts – something to take into account before we make this GA
| func setBaggageContextFromHeader(r *http.Request) (*http.Request, bool) { | ||
| baggageHeader := r.Header.Get("Baggage") | ||
| if baggageHeader == "" { | ||
| return r, false | ||
| } | ||
|
|
||
| b, err := baggage.Parse(baggageHeader) | ||
| if err != nil { | ||
| return r, false | ||
| } | ||
|
|
||
| ctx := baggage.ContextWithBaggage(r.Context(), b) | ||
| return r.WithContext(ctx), true | ||
| } | ||
|
|
There was a problem hiding this comment.
I agree that parsing the baggage in this middleware might be doing too much.
However, I assume parsing does provide a safeguard by validating and normalizing the baggage(?). What if we:
- Keep the current implementation
- Add a comment explaining this reasoning and the assumption of parsed baggage
- Offer a separate, optional middleware for baggage parsing
This PR modifies the existing k6 middleware to also add the OTEL baggage to the request context. This makes it easier for users to pass the baggage along when they make subrequests.
It also adds a few more tests and does some code refactoring.