feat(aws-serverless): Replace OTel Lambda instrumentation with handler redirection#22079
Conversation
…r redirection Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
andreiborza
requested review from
JPeer264,
Lms24,
chargome and
mydea
and removed request for
a team and
JPeer264
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2d5a898. Configure here.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
The approach seems reasonable to me, thanks for explaining it offline!
Had some minor comments regarding conventions and attribute deprecations.
| return { file: basePath, format: 'cjs' }; | ||
| } | ||
| if (isFile(`${basePath}.js`)) { | ||
| return { file: `${basePath}.js`, format: hasTypeModulePackageJson(path.dirname(basePath)) ? 'esm' : 'cjs' }; |
There was a problem hiding this comment.
l: Do lambdas always have a package.json? Or IOW, could we run into issues here where users define an index.js file in ESM but we identify CJS?
There was a problem hiding this comment.
I think that's an issue indeed but that issue also exists for the user, i.e. AWS looks at it the same way. If there's a .js file but no package.json then it automatically treats it as CJS regardless of what's in the file.
| * @deprecated Removed from the stable semantic conventions; not present in `@sentry/conventions`, so vendored here. | ||
| * Removed from the stable semantic conventions; not present in `@sentry/conventions`, so vendored here. | ||
| */ | ||
| export const ATTR_FAAS_EXECUTION = 'faas.execution'; |
There was a problem hiding this comment.
m (but no blocker for this PR, since we already send the attribute today): We should add this to conventions. OR, there's already in OTEL faas.invocation.id which sounds like the replacement to this?
Either way, faas.execution should be added to conventions and depending on if we use a replacement, it needs to be deprecated.
There was a problem hiding this comment.
Right, actually OTel already replaced this in their latest instrumentation update to use faas.invocation.id. I'll add a TODO that we should switch to faas.invocation.id in v11 but keep it as is for now.
There was a problem hiding this comment.
And I'll add it as a deprecated attribute to the conventions :)
There was a problem hiding this comment.
Added to conventions in #22079 (comment), updated the comment in 079361e
| * Removed from the stable semantic conventions; not present in `@sentry/conventions`, so vendored here. | ||
| */ | ||
| export const ATTR_FAAS_ID = 'faas.id'; |
There was a problem hiding this comment.
m: I'm not sure what the best replacement for this is (but I guess there should be one since it was deprecated). But similarly to above, we should add this attribute to conventions and mark it as deprecated if there is a replacement.
There was a problem hiding this comment.
Replacement is https://getsentry.github.io/sentry-conventions/attributes/cloud/#cloud-resource_id. I'll note this for v11 but keep as is for v10.
There was a problem hiding this comment.
Added to conventions with getsentry/sentry-conventions#475 and updated comment in 3c049fc.
2 participants
What
Replaces the vendored
@opentelemetry/instrumentation-aws-lambda(module patching viaInstrumentationBase+ import-in-the-middle) with handler redirection.The idea is that the AWS runtime reads
_HANDLERonly after all--import/--requirepreloads (and thusSentry.init()) have run. So during init, theAwsLambdaintegration points_HANDLERat a Sentry shim and stores the original value inSENTRY_ORIGINAL_HANDLER. The runtime then loads the shim instead of the user's handler; the shim resolves the original handler with the same rules as the runtime, wraps it with the existing span logic andwrapHandler, and re-exports it.sequenceDiagram participant Preload as Node.js preload<br/>(--import .../awslambda-auto) participant RIC as Lambda runtime participant Shim as Sentry shim<br/>(run-lambda-handler.mjs) participant User as User handler module Note over Preload: Sentry.init() Preload->>Preload: SENTRY_ORIGINAL_HANDLER = "index.handler"<br/>_HANDLER = ".../run-lambda-handler.handler" RIC->>Shim: reads _HANDLER, await import() Shim->>User: resolves SENTRY_ORIGINAL_HANDLER,<br/>require() for CJS / import() for ESM User-->>Shim: original handler Shim-->>RIC: exports wrapped handler<br/>(span logic + wrapHandler, streaming symbols preserved) loop every invocation RIC->>Shim: wrapped handler(event, context) Shim->>User: original handler(event, context) User-->>Shim: result Shim-->>RIC: result (span ended, events flushed) endBecause the shim wraps the handler value (whatever the export resolves to), this also covers wrapped (e.g. middy), re-exported and streaming handlers. Behavior is unchanged (same spans, attributes, origins, mechanisms), verified by the unchanged SAM e2e assertions (Node 18/20, layer and npm install methods) and against a real Lambda function. Datadog (redirect handler) and New Relic (wrapper handler) use the same approach in their layers.
Why not orchestrion
The issue proposed orchestrion, but it does not fit the Lambda case:
node_modules; the handler is an arbitrary user file resolved from_HANDLER.middy(...),awslambda.streamifyResponse(...), re-exports and bundled output.Closes: #20908