Repository navigation
Conversation
size-limit report 📦
|
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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.
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 8639881. Configure here.
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>
| '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.
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.
| '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.
|
👋 @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! |
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