feat(server-utils): Add orchestrion aws-sdk channel integration core#22142
Open
andreiborza wants to merge 14 commits into
Open
feat(server-utils): Add orchestrion aws-sdk channel integration core#22142andreiborza wants to merge 14 commits into
andreiborza wants to merge 14 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
size-limit report 📦
|
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
from
6b85478 to
ca51438
Compare
andreiborza
changed the title
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.
Reviewed by Cursor Bugbot for commit 47a0d74. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
4 times, most recently
from
9510b15 to
61916ca
Compare
Adds the core of an OTel-free, orchestrion diagnostics-channel implementation of the aws-sdk (v3) instrumentation to `@sentry/server-utils`. The integration subscribes to the `orchestrion:<smithy-pkg>:send` channels the transform injects into the smithy `Client.prototype.send` (`@smithy/core`, `@smithy/smithy-client`, `@aws-sdk/smithy-client`) and emits a client rpc span per command (`rpc.system`/`rpc.method`/`rpc.service`, `cloud.region`, request id metadata), with a distinct `auto.aws.orchestrion.aws-sdk` origin. The per-service extension registry (span names, messaging/db/gen_ai attributes, trace propagation) starts empty; the services are ported one group at a time in follow-up PRs, mirroring the OTel integration's ServiceExtension contract. Registered in `channelIntegrations`, so it is reachable via `@sentry/node`'s `experimentalUseDiagnosticsChannelInjection()`. The `@sentry/aws-serverless` swap follows at the top of the stack. Part of #20946 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Peer channel integrations (mysql, ioredis, anthropic) set the op at span start. Default to rpc, matching what the exporter infers from rpc.service, and let service extensions override it via RequestMetadata.spanOp where inference yields a different op.
The return-value override from the vendored OTel path cannot work through a tracing channel: subscribers cannot replace the value the caller's promise resolves with, so the subscriber rightly ignores any return value. Type responseHook as void and document that response changes (e.g. wrapping a stream) must mutate response.data in place, which is what the extensions do.
The region provider resolves asynchronously while send proceeds; when send settled first (e.g. an early failure) the fire-and-forget backfill hit an already-ended span and cloud.region was lost. deferSpanEnd now keeps the span open until the region promise settles. In the common path the SDK awaits the region internally before sending, so the promise is long settled and nothing changes.
Span origins only allow letters, digits, underscores, and dots per segment; matches the auto.ai.orchestrion.google_genai precedent.
The span is already started at that point; a sync throw bubbling into the enclosing safe wrapper would discard the span without ending it, leaking an open span for that AWS call.
Commands with all-optional members can be constructed without an input (e.g. new ListBucketsCommand()); the OTel path traces those, so default the command input to an empty object instead of skipping the span. All service extensions already read the input defensively.
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
from
61916ca to
828e59f
Compare
Service hooks mutate commandInput (propagation headers, MessageAttributeNames, ClientContext); with a detached default those writes never reached the serialized request for input-less commands on older smithy clients.
andreiborza
requested review from
JPeer264,
isaacs and
mydea
and removed request for
a team
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+213
to
+215
| return true; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Bug: A race condition in deferSpanEnd for streaming AWS requests can cause the cloud.region attribute to be lost if region resolution is slower than the stream completion.
Severity: MEDIUM
Suggested Fix
Modify the deferSpanEnd function to ensure that for streaming requests, it also checks for pending region resolution, similar to how it handles non-streaming requests. This involves removing the early return for streams and allowing the logic to fall through to the check that defers the span end until the region promise settles.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/server-utils/src/integrations/tracing-channel/aws-sdk/index.ts#L213-L215
Potential issue: For streaming AWS SDK responses, a race condition can occur due to an
early return in the `deferSpanEnd` function. This function returns `true` for streaming
requests before checking if the AWS region resolution is still pending. If a streaming
operation completes and its corresponding span is ended before the asynchronous region
promise resolves, the subsequent attempt to set the `cloud.region` attribute on the span
will fail silently because the span is no longer recording. This results in the loss of
the `cloud.region` attribute in the telemetry data for these specific requests.
Did we get this right? 👍 / 👎 to inform future reviews.
JPeer264
approved these changes
Jul 13, 2026
|
|
||
| export function removeSuffixFromStringIfExists(str: string, suffixToRemove: string): string { | ||
| const suffixLength = suffixToRemove.length; | ||
| return str?.slice(-suffixLength) === suffixToRemove ? str.slice(0, str.length - suffixLength) : str; |
Member
There was a problem hiding this comment.
super-l: Since you already using .slice(-suffixLength) you could use the following to be consistent of the usage:
Suggested change
| return str?.slice(-suffixLength) === suffixToRemove ? str.slice(0, str.length - suffixLength) : str; | |
| return str?.slice(-suffixLength) === suffixToRemove ? str.slice(0, -suffixLength) : str; |
| } | ||
| }, | ||
| { | ||
| "files": ["**/integrations/tracing-channel/aws-sdk/**/*.ts"], |
Member
There was a problem hiding this comment.
q: Any chance there is a way to not disable these entirely?
| * `@opentelemetry/semantic-conventions`), inlined here so the integration stays free of OTel deps. | ||
| * Attributes that exist in `@sentry/conventions/attributes` are imported from there instead; | ||
| * TODO(aws-sdk): the active attributes below are being added to sentry-conventions and should move | ||
| * to `@sentry/conventions/attributes` imports once a release containing them ships. |
Member
There was a problem hiding this comment.
to
@sentry/conventions/attributesimports once a release containing them ships
Is there already a plan / PR to add them?
| public constructor() { | ||
| // Per-service extensions, keyed by the client's `serviceId` (e.g. `'S3'`). Services without a | ||
| // registered extension still get the base rpc span from the subscriber. | ||
| this._services = new Map(); |
Member
There was a problem hiding this comment.
l/q: Wouldn't the following be the same? Then we could get rid of the constructor
export class ServicesExtensions implements ServiceExtension {
#services = new Map<string, ServiceExtension>();
...
}| kind: requestMetadata.spanKind ?? SPAN_KIND.CLIENT, | ||
| // `rpc` matches what the exporter infers from `rpc.service` for the OTel aws-sdk spans; | ||
| // service extensions override it where inference yields a different op (DynamoDB: `db`). | ||
| op: requestMetadata.spanOp ?? 'rpc', |
Member
There was a problem hiding this comment.
l: Maybe it is better to also guard against empty strings or general falsy values? Or is this not an issue here?
Suggested change
| op: requestMetadata.spanOp ?? 'rpc', | |
| op: requestMetadata.spanOp || 'rpc', |
Co-authored-by: Jan Peer Stöcklmair <jan.peer@sentry.io>
JPeer264
reviewed
Jul 13, 2026
| }, | ||
| { | ||
| channelName: 'send', | ||
| module: { name: '@smithy/smithy-client', versionRange: '>=1.0.3', filePath: 'dist-cjs/index.js' }, |
Member
There was a problem hiding this comment.
Suggested change
| module: { name: '@smithy/smithy-client', versionRange: '>=1.0.3', filePath: 'dist-cjs/index.js' }, | |
| module: { name: '@smithy/smithy-client', versionRange: '>=1.0.3 <5', filePath: 'dist-cjs/index.js' }, |
Comment on lines
+212
to
+214
| if (requestMetadata?.isStream && !failed) { | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: An early return for streaming AWS SDK responses bypasses logic to defer ending the span, causing the cloud.region attribute to be lost if region resolution is slow.
Severity: MEDIUM
Suggested Fix
Ensure that the logic to defer ending the span until the region promise settles is also applied to streaming requests. This can be done by checking for a pending region and deferring before the early return for streams, or by making the service extension responsible for awaiting the region promise before ending the span.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/server-utils/src/integrations/tracing-channel/aws-sdk/index.ts#L212-L214
Potential issue: For streaming AWS SDK responses, an early return in `deferSpanEnd` at
lines 212-214 bypasses the logic that defers ending a span until the `cloud.region` is
resolved. When a service extension (e.g., for Bedrock) later ends the span after the
stream is consumed, the region promise might still be pending. If the promise resolves
after the span has ended, the `setAttribute` call to add the region will be ignored,
resulting in the `cloud.region` attribute being omitted from the final span. This issue
will manifest once streaming service extensions are implemented as planned in follow-up
work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The 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 a client rpc span per command (rpc.system/rpc.method/rpc.service,cloud.region, request id metadata), with a distinctauto.aws.orchestrion.aws_sdkorigin.The per-service extension registry (span names, messaging/db/gen_ai attributes, trace propagation) starts empty and mirrors the OTel integration's
ServiceExtensioncontract; the services are ported one group at a time in the follow-up PRs of this stack.The OTel instrumentation was only added to the
@sentry/aws-serverlessSDK, but it is useable in other SDKS so the orchestrion version lives in server-utils now.User-facing differences to the vendored OTel instrumentation
auto.aws.orchestrion.aws_sdkinstead ofauto.otel.awsReceiveMessageparent to the surrounding span instead of the receive span; receive-to-consumer linkage uses span links (messaging PR)sentry-trace/baggagemessage attributes instead of W3C headers (messaging PR)aws.request.id/aws.request.extended_idwhere the OTel path missed them ($metadatafallback)Part of #20946