feat(server-utils): Migrate @opentelemetry/instrumentation-aws-sdk to orchestrion#22138
feat(server-utils): Migrate @opentelemetry/instrumentation-aws-sdk to orchestrion#22138andreiborza wants to merge 2 commits into
@opentelemetry/instrumentation-aws-sdk to orchestrion#22138Conversation
…to orchestrion Migrates the aws-sdk (v3) instrumentation from the OTel `InstrumentationBase` patcher (vendored in `@sentry/aws-serverless`) to an orchestrion diagnostics-channel listener. The channel-based integration subscribes to `orchestrion:<smithy-pkg>:send` (injected into the smithy `Client.prototype.send`) and emits the same spans as the OTel integration for all services (S3, DynamoDB, SQS, SNS, Lambda, Kinesis, SecretsManager, StepFunctions, Bedrock) with a distinct `auto.aws.orchestrion.aws-sdk` origin. Registered in `channelIntegrations` so it is reachable via `@sentry/node`'s `experimentalUseDiagnosticsChannelInjection()`, and swapped in for the OTel `Aws` integration in `@sentry/aws-serverless` under the same opt-in. closes #20946 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| // Streaming responses end the span when their wrapped stream is consumed (see | ||
| // bedrock-runtime); the helper must not end it on `send` settling. Errors always end here. | ||
| return !!requestMetadata?.isStream && !failed; |
There was a problem hiding this comment.
Stream spans leak on hook failure
Medium Severity
For Bedrock streaming commands, deferSpanEnd always skips ending the span when isStream is set and the request did not fail, even if the wrapped responseHook threw inside safe. The AWS call still succeeds, but no wrapper runs to call span.end(), leaving a non-finishing client span.
Reviewed by Cursor Bugbot for commit 86e221a. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ 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 46c35c3. Configure here.
|
|
||
| if (request.commandInput?.body) { | ||
| const requestBody = JSON.parse(request.commandInput.body); | ||
| if (modelId.includes('amazon.titan')) { |
There was a problem hiding this comment.
Unchecked modelId before includes
Low Severity
In InvokeModel pre-span handling, when commandInput.body is present the code calls modelId.includes(...) without ensuring modelId is defined. A body-only invoke can throw inside the orchestrion safe() wrapper, so instrumentation skips span creation while the AWS call still runs.
Reviewed by Cursor Bugbot for commit 46c35c3. Configure here.
|
|
||
| if (Object.keys(attributes).length + headerKeys.length <= MAX_MESSAGE_ATTRIBUTES) { | ||
| for (const key of headerKeys) { | ||
| (attributes as AwsSdkContextObject)[key] = { DataType: 'String', StringValue: headers[key] } as any; |
There was a problem hiding this comment.
as any lacks explanation comment
Low Severity
New SDK code assigns SQS/SNS message attribute entries with an as any cast and no comment explaining why a narrower type is not used, which violates the project’s PR review rules for any in production SDK source.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 46c35c3. Configure here.
|
Superseded by a stacked split for easier review:
Same change, split into reviewable layers. |
1 participant
Migrates the aws-sdk (v3) instrumentation from the OpenTelemetry
InstrumentationBasepatcher (vendored in@sentry/aws-serverless) to an orchestrion diagnostics-channel listener, following the mysql (#20900) and graphql (#21885) precedent.The channel-based integration subscribes to the
orchestrion:<smithy-pkg>:sendchannels the transform injects into the smithyClient.prototype.send(@smithy/core,@smithy/smithy-client,@aws-sdk/smithy-client), and emits the same spans as the OTel integration for every service (S3, DynamoDB, SQS, SNS, Lambda, Kinesis, SecretsManager, StepFunctions, Bedrock), with a distinctauto.aws.orchestrion.aws-sdkorigin.@sentry/server-utilsand are registered inchannelIntegrations, so the integration is reachable via@sentry/node'sexperimentalUseDiagnosticsChannelInjection().@sentry/aws-serverless(where the OTelAwsintegration ships today) swaps in the channel version under the same opt-in, via a smallapplyDiagnosticsChannelInjectionIntegrationshelper extracted from@sentry/node.sentry-trace/baggageheaders instead of OTelpropagation.inject.Testing reuses the existing nock-mocked
aws-integrationandaws-integration-streamednode-integration-test suites: they already run under theINJECT_ORCHESTRIONCI matrix, so the diagnostics-channel path is asserted for identical spans across both smithy stacks (latest + legacy3.1041.0) and ESM/CJS. Only the expected origin is parametrized.closes #20946