Repository navigation
feat(node): Add awsLambdaWebIntegration for Lambda Web Functions - #25355
Conversation
size-limit report 📦Compared: a800abe (PR test merge using develop at b0560b7) Against: b0560b7 (develop) · base run #64377 ✅ Baseline matches: this diff contains only this PR's changes.
|
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 'faas.max_memory': lambdaContext.memory_size, | ||
| 'aws.lambda.initialization_type': lambdaContext.initialization_type, |
There was a problem hiding this comment.
Do we already have those attributes in conventions?
There was a problem hiding this comment.
Def wanna make these not hard-coded once that lands and probably, wait on this until then. If we need to land this first for some reason, then we should open a tracking issue to make sure it's not forgotten.
There was a problem hiding this comment.
revisiting this, has this been released yet/can we use this yet?
There was a problem hiding this comment.
merged, but not yet released. I'll see if we can do a conventions release soon
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
The main blocker is waiting for the conventions to land, obviously. This is a good approach overall.
| const cloudContext: SpanAttributes = { | ||
| [CLOUD_PROVIDER]: 'aws', | ||
| [CLOUD_PLATFORM]: 'aws_lambda', | ||
| [CLOUD_REGION]: process.env.AWS_REGION, | ||
| [CLOUD_ACCOUNT_ID]: process.env.AWS_ACCOUNT_ID, | ||
| }; |
There was a problem hiding this comment.
cloud_resource here overlaps nodeContextIntegration, and the result depends on integration order, which could be brittle.
nodeContextIntegration already detects AWS through AWS_REGION (packages/node/src/integrations/context.ts line 531) and sets cloud.platform to process.env.AWS_EXECUTION_ENV (for example AWS_Lambda_nodejs24.x). This PR sets cloud.platform to aws_lambda. Both integrations use "existing value wins" merging ({ ...defaults, ...event.contexts.cloud_resource } in processEvent, and safeSetSpanJSONAttributes in processSegmentSpan). So the integration that runs first wins, for events and for spans.
Today awsLambdaWebIntegration() sits just before nodeContextIntegration() in packages/node/src/sdk/index.ts line 70, so aws_lambda wins. The integration test proves this, because it sets AWS_EXECUTION_ENV. But nothing documents the dependency. If a user builds the list with integrations: defaults => [...] or defaultIntegrations: false and puts this integration after nodeContextIntegration, cloud.platform silently becomes AWS_Lambda_nodejs24.x-lite. A later reorder of the defaults would do the same, and only the integration test would catch it.
This PR also has two places that decide "what cloud am I in" for the same process.
Suggestion: move the Web Function cloud detection into getCloudResourceContext() in packages/node/src/integrations/context.ts (when AWS_LAMBDA_HTTP_ENDPOINT is set, return cloud.platform: 'aws_lambda' plus cloud.account.id). Then awsLambdaWebIntegration only handles the aws.lambda context and the per-request data, and the order no longer matters. Or, I guess, we could add a short comment at the default-list entry that says it must come before nodeContextIntegration, and why.
There was a problem hiding this comment.
good point, i did some more digging and came to the conclusion that imo we actually don't need to handle web function web function specially here for two reasons:
cloud.platformshould just beaws_lambdafor web functions and regular lambda functions instead of the full runtime. This is both the example value in conventions and how we set it in the aws-serverless SDK. SinceAWS_REGIONbeing present doesn't necessarily mean that the code's being executed in Lambda, i added a check whetherAWS_EXECUTION_ENVstarts withAWS_Lambdaand setcloud.platform: 'aws_lambda'if so, otherwise fall back to the the full env var value (maybe we should handle other AWS execution envs (ECS?) in a follow-up).AWS_ACCOUNT_IDisn't actually set by default on Lambda (web function or not), that was an oversight from my end. In the aws-serverless SDK we extract it from the function's ARN, which we we don't have available here.
| 'faas.max_memory': lambdaContext.memory_size, | ||
| 'aws.lambda.initialization_type': lambdaContext.initialization_type, |
There was a problem hiding this comment.
Def wanna make these not hard-coded once that lands and probably, wait on this until then. If we need to land this first for some reason, then we should open a tracking issue to make sure it's not forgotten.
| if (getClient() !== client) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This guard doesn't actually do anything.
client.on('httpServerRequest', ...) registers the callback on this client only. The only emitter (packages/node/src/integrations/http/httpServerIntegration.ts line 127) calls emit on getClient(). So the callback only runs when getClient() === client. httpServerSpansIntegration subscribes to the same hook without a guard. Coverage confirms that line 40 is never hit.
| if (getClient() !== client) { | |
| return; | |
| } |
| const scope = getIsolationScope(); | ||
| const requestContext: { cold_start: boolean; aws_request_id?: string; endpoint_name?: string } = { | ||
| ...scope.getScopeData().contexts['aws.lambda'], | ||
| cold_start: coldStart, | ||
| }; | ||
| if (typeof requestId === 'string') { | ||
| requestContext.aws_request_id = requestId; | ||
| } else { | ||
| delete requestContext.aws_request_id; | ||
| } | ||
| if (typeof endpointName === 'string') { | ||
| requestContext.endpoint_name = endpointName; | ||
| } else { | ||
| delete requestContext.endpoint_name; | ||
| } | ||
| scope.setContext('aws.lambda', requestContext); |
There was a problem hiding this comment.
Context values of undefined are dropped when the event is normalized, and the processSegmentSpan code already checks typeof ... === 'string'. So I think we can do this with a bit less code:
| const scope = getIsolationScope(); | |
| const requestContext: { cold_start: boolean; aws_request_id?: string; endpoint_name?: string } = { | |
| ...scope.getScopeData().contexts['aws.lambda'], | |
| cold_start: coldStart, | |
| }; | |
| if (typeof requestId === 'string') { | |
| requestContext.aws_request_id = requestId; | |
| } else { | |
| delete requestContext.aws_request_id; | |
| } | |
| if (typeof endpointName === 'string') { | |
| requestContext.endpoint_name = endpointName; | |
| } else { | |
| delete requestContext.endpoint_name; | |
| } | |
| scope.setContext('aws.lambda', requestContext); | |
| const scope = getIsolationScope(); | |
| scope.setContext('aws.lambda', { | |
| ...scope.getScopeData().contexts['aws.lambda'], | |
| cold_start: coldStart, | |
| aws_request_id: typeof requestId === 'string' ? requestId : undefined, | |
| endpoint_name: typeof endpointName === 'string' ? endpointName : undefined, | |
| }); |
| modulesIntegration, | ||
| contextLinesIntegration, | ||
| nodeContextIntegration, | ||
| awsLambdaWebIntegration, |
There was a problem hiding this comment.
I noticed that in packages/elysia/src/index.ts line 77: Elysia re-exports nodeContextIntegration from @sentry/bun but not awsLambdaWebIntegration.
It's being added to @sentry/bun (so the consistentExports check passes), but @sentry/elysia, which re-exports a list from @sentry/bun, does not get it. consistentExports.ts doesn't check Elysia, so CI won't catch this. Of course, they're node-only, so it kind of doesn't make sense anyway.
Suggestion: either add awsLambdaWebIntegration to the Elysia export list for consistency, or (maybe more sensible? since they're node-only) add it to Bun's ignoreExports in dev-packages/e2e-tests/test-applications/node-exports-test-app/scripts/consistentExports.ts and remove it from here.
There was a problem hiding this comment.
did the more sensible option and removed the export from bun (and added it to ignoreExports)
|
👋 @mydea, @chargome, @andreiborza — Please review this PR when you get a chance! |
1 similar comment
|
👋 @mydea, @chargome, @andreiborza — Please review this PR when you get a chance! |
c5e823d to
bf3b43e
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bf3b43e. Configure here.
| 'cloud.platform': process.env.AWS_EXECUTION_ENV, | ||
| [CLOUD_PROVIDER]: 'aws', | ||
| [CLOUD_REGION]: process.env.AWS_REGION, | ||
| [CLOUD_PLATFORM]: executionEnv?.startsWith('AWS_Lambda') ? 'aws_lambda' : executionEnv, |
There was a problem hiding this comment.
Silent cloud.platform value change
Medium Severity
nodeContextIntegration now writes cloud.platform as aws_lambda whenever AWS_EXECUTION_ENV starts with AWS_Lambda, instead of the previous raw value such as AWS_Lambda_nodejs24.x. This applies to every Node Lambda app, not only Web Functions, so existing queries and alerts keyed on the old value can stop matching, and the diff has no migration note. Flagged because the review rules call out silent payload changes without a migration callout.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit bf3b43e. Configure here.
There was a problem hiding this comment.
seems like a reasonable comment to look into I'd say!
There was a problem hiding this comment.
yes, that's fair. I laid out here why I think just aws_lambda is the correct value here in all cases, not just for web functions. Do you think we can get away by labeling this as a fix (to align with conventions/otel)? Otherwise we can do it for web functions only now and fix it for other Node apps in v12.
There was a problem hiding this comment.
briefly discussed with @Lms24 offline and we agreed that it's okay to fix this here. OTel conventions explicitly say that
cloud.platformhas the following list of well-known values [list includesaws_lambda]. If one of them applies, then the respective value MUST be used; otherwise, a custom value MAY be used.
Optional E2E testsSome optional checks were not successful. They do not block the merge. 2 not successful, 44 successful. See the run.
Successful checks
|
| return { | ||
| name: 'AwsLambdaWeb', | ||
| setup(client) { | ||
| if (!enabled) return; |
There was a problem hiding this comment.
l: this seems fine, but we could also just short-circuit this above, and do e.g.:
if (!enabled) {
return { name: AWS_LAMBDA_WEB_INTEGRATION_NAME };
}
return {
// ... remaining stuff here
}
bf3b43e to
071523f
Compare
|
One or more gzipped bundles increased by more than 500 bytes. If this increase is intentional, add the Accept Bundlesize Increase label to this PR to rerun and accept the check. |
| let requestHandled = false; | ||
|
|
||
| const _awsLambdaWebIntegration = (() => { | ||
| if (!process.env.AWS_LAMBDA_HTTP_ENDPOINT) { |
There was a problem hiding this comment.
let's add a comment here about the logic here. which is, if I understand correctly, that we cannot hook into this any better, so this has to be handled via client.on('httpServerRequest'), which would run for each server. so in order to avoid the overhead of having this run everywhere, we opt out here?
There was a problem hiding this comment.
yes exactly, it's just a regular HTTP server with the request metadata living in request headers. added a comment to explain.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Nice addition! The integration looks solid and makes good use of scope and client hooks. Previous review comments are addressed and I only had some nits left 😅
| import { expect, vi } from 'vitest'; | ||
| import { createEsmAndCjsTests } from '../../../utils/runner'; | ||
|
|
||
| createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { |
There was a problem hiding this comment.
l: I rarely critique style, so sorry in advance 😅 I think this test could benefit from a few line breaks to make it a bit easier to understand what belongs directly together.
There was a problem hiding this comment.
that's very fair, i added a few line breaks 😄
| }), | ||
| ]); | ||
| await running.completed(); | ||
| for (const { request, collected } of spans) { |
There was a problem hiding this comment.
l: should we assert on the number of segment spans / traces? I believe it should be 2, correct?
There was a problem hiding this comment.
yes should be 2, added the assertion
mydea
left a comment
mydea
left a comment
There was a problem hiding this comment.
small nit about maybe adding a comment, other than this looks good!
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
Co-Authored-By: GPT-6 <codex@openai.com>
de36fe9 to
c0111af
Compare
This adds a new
awsLambdaWebIntegrationto the Node SDK for instrumenting Lambda Web Functions. Web Functions run standard Node HTTP servers like Express or Hono, without Lambda-specific handlers or adapters.Enriches event contexts and segment spans with AWS metadata, request IDs, endpoint names, and cold-start information.
Closes #25317
conventions PR for new attributes: getsentry/sentry-conventions#715