-
Notifications
You must be signed in to change notification settings - Fork 596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lambda: use _X_AMZN_TRACE_ID trace context as a Link not parent #3428
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ package xrayconfig // import "go.opentelemetry.io/contrib/instrumentation/github | |
|
||
import ( | ||
"context" | ||
"os" | ||
|
||
lambdadetector "go.opentelemetry.io/contrib/detectors/aws/lambda" | ||
"go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-lambda-go/otellambda" | ||
|
@@ -26,9 +25,10 @@ import ( | |
sdktrace "go.opentelemetry.io/otel/sdk/trace" | ||
) | ||
|
||
// TODO: Currently does nothing but should support pulling the propagated | ||
// trace context from attributes depending on the type of Lambda event | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I support removing the 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is lambda specific. If it continued to extract the environment variable then there would have to be a new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Aneurysm9 can this be resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recommend that we not remove this functionality, but should remove it from the There are two reasons I see for this change to the PR:
|
||
} | ||
|
||
// NewTracerProvider returns a TracerProvider configured with an exporter, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
, withWithLinks
accepting those.Returning
nil
or*trace.Link
felt off (especially since I didn't see a way to not have to do onestart
in the first branch ofif 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?