fix(aws-serverless): Extract sentry trace data from handler context over event#13266
fix(aws-serverless): Extract sentry trace data from handler context over event#13266andreiborza merged 4 commits intodevelopfrom
context over event#13266Conversation
size-limit report 📦
|
| hasBundles: true, | ||
| packageSpecificConfig: { | ||
| // Used for our custom eventContextExtractor | ||
| external: ['@opentelemetry/api'], |
There was a problem hiding this comment.
just out of curiosity, why is this needed?
There was a problem hiding this comment.
Package build structure isn't right without this. It pulls this in under its own node_modules folder.
| * Sentry's context. This should usually be `true` when | ||
| * using Sentry to instrument Lambdas. |
There was a problem hiding this comment.
| * Sentry's context. This should usually be `true` when | |
| * using Sentry to instrument Lambdas. | |
| * Sentry's context. Defaults to `true`, in order for Sentry trace propagation to take precedence, | |
| * but can be disabled if you want to AWS propagation to take precedence. |
or something like this?
packages/aws-serverless/src/sdk.ts
Outdated
| import { awsIntegration } from './integration/aws'; | ||
| import { awsLambdaIntegration } from './integration/awslambda'; | ||
| import { markEventUnhandled } from './utils'; | ||
| import { getTraceData, markEventUnhandled } from './utils'; |
There was a problem hiding this comment.
m: Can we change the name here, as it conflicts with getTraceData we have in core, making this possibly a bit confusing 😅 E.g. getAwsTraceData or something like this?
| const customContext: Record<string, unknown> = context.clientContext.Custom; | ||
| const sentryTrace = isString(customContext['sentry-trace']) ? customContext['sentry-trace'] : undefined; | ||
|
|
||
| if (sentryTrace) { |
There was a problem hiding this comment.
m: It will probably not matter in 99% of cases, but let's just split this into two if-blocks. So basically:
const sentryTrace = ...;
const baggage = ...;
if(sentryTrace) {
traceData['sentry-trace'] = sentryTrace;
}
if(baggage) {
traceData.baggage = baggage;
}I think this is slightly more robust :)
There was a problem hiding this comment.
I was thinking of that, but wouldn't we potentially end up with a mix of wrong pairs, e.g. both event and context have baggage but only event has sentry-trace. So we end up with sentry-trace from event and baggage from context.
I don't know if that's a realistic usecase tho. In core, we also throw away baggage if sentryTrace is not valid.
There was a problem hiding this comment.
right, then let's keep it this way, all good! 👍
|
|
||
| /** | ||
| * Extracts sentry trace data from the handler `context` if available and falls | ||
| * back to the `event`. |
There was a problem hiding this comment.
Maybe we can also add a short note here when this would be on context and when on event?
There was a problem hiding this comment.
I added an explanation, but tbh this is pretty wide open. I think different AWS services make use of different event/context usage. Hope this adds a bit more info?
mydea
left a comment
mydea
left a comment
There was a problem hiding this comment.
left some notes, more about details - overall great work! Big improvement :)
2 participants
Currently, the AWS otel integration (and our
wrapHandlerfallback) try to extract sentry trace data from theeventobject passed to a Lambda call. The aws-sdk integration, however, places tracing data ontocontext.clientContext.Custom.This PR adds a custom
eventContextExtractorthat attempts extracting sentry trace data from thecontext, with a fallback toeventto enable distributed tracing among Lambda invocations.Traces are now connected. Here an example:
Lambda-AcallingLambda-B:Lambda-B:Closes: #13146