[aws-lambda] POC - Request for Comments - Add support to carrier pre-processing in aws-lambda instrumentation #9300
Conversation
|
@tylerbenson could you help reviewing this |
rapphil
changed the title
rapphil
changed the title
|
I'm trying to get clarification/confirmation from the TC. Once I get that, I'll review. |
Can you provide more information on what clarification/confirmation you are seeking? |
|
So there's a few fundamental decisions that were made in OTEL:
Given these two, there's one central problem I see with this PR: We should be preparing users for the new default parenting of spans in message passing scenarios to be links, not spans. We need to be working on a transition period and configuraiton to help users perserve the behavior they have today, but also let them know the default will be changing in OTEL (OTEP 220 guarantees this). Given we are talking about breaking changes for users, I'd rather not churn how the instrumentation works any more than is strictly necessary. This PR does not move in the direction of OTEP 220, and I'm concerned the new environment variable that's added has the wrong default for the future of OTEL. |
Signed-off-by: Raphael Silva <[email protected]>
Signed-off-by: Raphael Silva <[email protected]>
Signed-off-by: Raphael Silva <[email protected]>
Signed-off-by: Raphael Silva <[email protected]>
Signed-off-by: Raphael Silva <[email protected]>
rapphil
force-pushed
the
rapphil-poc-semantic-conventions-277
branch
from
7e12eda to
833392c
Compare
Signed-off-by: Raphael Silva <[email protected]>
...opentelemetry/instrumentation/awslambdacore/v1_0/internal/AwsLambdaFunctionInstrumenter.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/instrumentation/awslambdacore/v1_0/internal/CarrierEnricher.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Raphael Silva <[email protected]>
| return new AwsLambdaFunctionInstrumenter( | ||
| openTelemetry, | ||
| InstrumenterBuilder<AwsLambdaRequest, Object> otelInstrumenterBuilder = | ||
| Instrumenter.builder( | ||
| openTelemetry, | ||
| "io.opentelemetry.aws-lambda-events-2.2", | ||
| AwsLambdaEventsInstrumenterFactory::spanName) | ||
| .addAttributesExtractor(new AwsLambdaFunctionAttributesExtractor()) | ||
| .addAttributesExtractor(new ApiGatewayProxyAttributesExtractor()) | ||
| .buildInstrumenter(SpanKindExtractor.alwaysServer())); | ||
| .addAttributesExtractor(new ApiGatewayProxyAttributesExtractor()); | ||
|
|
||
| return new AwsLambdaFunctionInstrumenter( | ||
| openTelemetry, otelInstrumenterBuilder.buildInstrumenter(SpanKindExtractor.alwaysServer())); |
There was a problem hiding this comment.
I think can revert these changes now (just to clean up the diff and make the POC review easier for others)
| return new AwsLambdaFunctionInstrumenter( | ||
| openTelemetry, | ||
| InstrumenterBuilder<AwsLambdaRequest, Object> otelInstrumenterBuilder = | ||
| Instrumenter.builder( | ||
| openTelemetry, | ||
| "io.opentelemetry.aws-lambda-core-1.0", | ||
| AwsLambdaFunctionInstrumenterFactory::spanName) | ||
| .addSpanLinksExtractor(new AwsXrayEnvSpanLinksExtractor()) | ||
| .addAttributesExtractor(new AwsLambdaFunctionAttributesExtractor()) | ||
| .buildInstrumenter(SpanKindExtractor.alwaysServer())); | ||
| .addAttributesExtractor(new AwsLambdaFunctionAttributesExtractor()); | ||
|
|
||
| return new AwsLambdaFunctionInstrumenter( | ||
| openTelemetry, otelInstrumenterBuilder.buildInstrumenter(SpanKindExtractor.alwaysServer())); |
Signed-off-by: Raphael Silva <[email protected]>
|
|
||
| private static final String AWS_TRACE_ENV_VAR = "_X_AMZN_TRACE_ID"; | ||
| private static final String AWS_TRACE_PROPERTY = "com.amazonaws.xray.traceHeader"; | ||
| private static final String AWS_TRACE_PROPAGATOR_KEY = "x-amzn-trace-id"; |
There was a problem hiding this comment.
my preference would be for this to be a separate key in the carrier, e.g. x-amzn-env-trace-id or x-amzn-vended-trace-id (instead of overwriting the AwsLambdaRequest header), so that the propagator chain can have access to all of the different propagated trace contexts
but this is a question that needs to be answered on the spec side
|
is this PR still relevant? |
|
I don't think so. |
This PR is a POC for the proposal FaaS: Change requirements regarding handling of AWS Lambda-provided SpanContext
In this PR we:
extractoperation of the global propagators.* Adds a new optionotel.instrumentation.aws-lambda.link-xray-traces(also as env varOTEL_INSTRUMENTATION_AWS_LAMBDA_LINK_XRAY_TRACES) to allow users to include X-Ray traces originated from the lambda environment as span links.p.s.: there is no way to configure the global context propagators to include the X-Ray propagator to fully validate this change.