lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent #3428
Conversation
tsloughter
force-pushed
the
lambda-xray-env
branch
from
68ca172 to
94b3e70
Compare
|
Note that Another option is to remove |
tsloughter
force-pushed
the
lambda-xray-env
branch
5 times, most recently
from
6037833 to
05b949c
Compare
| func xrayEventToCarrier([]byte) propagation.TextMapCarrier { | ||
| xrayTraceID := os.Getenv("_X_AMZN_TRACE_ID") | ||
| return propagation.HeaderCarrier{"X-Amzn-Trace-Id": []string{xrayTraceID}} | ||
| return propagation.HeaderCarrier{} |
There was a problem hiding this comment.
I don't think this change is necessary. This package provides a set of helpers for configuring the Lambda instrumentation with opinionated defaults, but is not required to use the instrumentation. Leaving it as-is can minimize the impact to users who already have a working setup with these defaults while still having the expected linking behavior.
There was a problem hiding this comment.
But having this become the parent was part of the confusion people were seeing that led to changing it to a link.
There was a problem hiding this comment.
I support removing the _X_AMZN_TRACE_ID environment variable reference here because it doesn't conform to the updated spec.
Eventually this code should be updated to actually provide a carrier from the input, but I don't think that should preclude removing the environment variable usage.
There was a problem hiding this comment.
This is not the Lambda instrumentation and does not need to conform to any specification. It is a helper provided to make it easier for users to have an experience that aligns with that of using X-Ray.
There was a problem hiding this comment.
It is lambda specific. If it continued to extract the environment variable then there would have to be a new eventToCarrier created to handle lambda events and ignore the environment variable and this one would be unused?
There was a problem hiding this comment.
I recommend that we not remove this functionality, but should remove it from the WithRecommendedOptions.
There are two reasons I see for this change to the PR:
- This allows a path for current users to keep current functionality.
- There does seem to be a missing propagation here. I'm not fluent enough if X-Ray to know what should be in the X-Amzn-Trace-Id header (would the local trace-id and span id work?) but it would seem without it all requests made would be orphaned.
tsloughter
force-pushed
the
lambda-xray-env
branch
from
05b949c to
67d1ded
Compare
| @@ -56,6 +57,23 @@ func newInstrumentor(opts ...Option) instrumentor { | |||
| resAttrs: []attribute.KeyValue{}} | |||
| } | |||
|
|
|||
| func xrayEnvToLinks() []trace.Link { | |||
There was a problem hiding this comment.
Not sure if this is idiomatic to return a list of one item or an empty list.
In some languages I'd definitely do it this way, in others I'd return null and others I'd return a Maybe Link, with WithLinks accepting those.
Returning nil or *trace.Link felt off (especially since I didn't see a way to not have to do one start in the first branch of if link != nil and another in the else branch), but maybe that is the way to go to avoid an unnecessary creation of an empty list?
See open-telemetry/opentelemetry-specification#3166