feat(server-utils): Add orchestrion aws-sdk channel integration core#22142
feat(server-utils): Add orchestrion aws-sdk channel integration core#22142andreiborza wants to merge 20 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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
5 times, most recently
from
61916ca to
828e59f
Compare
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}
| } | ||
| }, | ||
| { | ||
| "files": ["**/integrations/tracing-channel/aws-sdk/**/*.ts"], |
There was a problem hiding this comment.
q: Any chance there is a way to not disable these entirely?
There was a problem hiding this comment.
I tightened this a bit, but kept no-explicit-any because it's used quite a bit.
| * `@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. |
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?
There was a problem hiding this comment.
Yep, they were added. We need to upgrade the conv package and switch over, but I'll do that at the end of this stack.
| 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(); |
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>();
...
}There was a problem hiding this comment.
Sure, this code was taken verbatim from the vendored instrumentation but I'll adjust.
There was a problem hiding this comment.
# apparently uses an extra lookup so I just changed it to a private field in a3773ef
| 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', |
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?
| op: requestMetadata.spanOp ?? 'rpc', | |
| op: requestMetadata.spanOp || 'rpc', |
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
from
88e502d to
cdb3108
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
LGTM.
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.
I love this :) We should make it available to other platforms.
| expressIntegration: expressChannelIntegration, | ||
| graphqlIntegration: graphqlDiagnosticsChannelIntegration, | ||
| kafkajsIntegration: kafkajsChannelIntegration, | ||
| awsIntegration: awsChannelIntegration, |
There was a problem hiding this comment.
low/comment: I assume that eventually (once the stack completes) this will have coverage of the whole current awsIntegration. But, just a note to keep in mind, since as of this PR it's only partial, but wiring it in will fully disable the existing OTel integration, we might want to hold off on landing these (or at least, shipping them to production) until the full integration is covered.
Another approach would be to land the components in pieces, but only wire it in here once it's complete. If there's delays getting it finished, that might be safer, but tbh, just landing them all together in advance of a release is probably also fine.
There was a problem hiding this comment.
Yep, functionality will complete by the end of the gh stack and I'll only merge them in as a whole stack.
Good point though, I didn't consider that it will completely disable the otel integration without feature parity if we merge as is.
Anyway, I'll keep this open and merge the stack as a whole.
| // The orchestrion aws-sdk channel integration has no service extensions yet (empty registry), | ||
| // so it can't emit the service-specific attributes asserted here. Stay on the OTel path until | ||
| // the service extensions land in a follow-up. | ||
| { additionalDependencies, injectOrchestrion: false }, |
There was a problem hiding this comment.
low: I haven't seen the follow-up yet, so presumably it will be fully tested once it all lands (and if not, we can review/address it there), but this feels a little risky. I wouldn't gate on it, but it's a risk to make sure to address later.
Maybe overkill, but is it possible to add some tests that at least exercise the new code paths, even if it's just unit tests?
There was a problem hiding this comment.
I'll merge this entire integration as a stack at once so I don't think it's worth the effort to implement here when the follow up PRs in the same stack are going to implement the actual tests.
If we were to merge this as a standalone I agree.
| // open span). | ||
| let regionResult: string | Promise<string> | undefined; | ||
| try { | ||
| regionResult = clientConfig?.region?.(); |
There was a problem hiding this comment.
I think this is fine, but an interesting wrinkle that might be worth calling out in the docs.
Since the AWS client itself calls config.region() to get the region, now we're invoking that twice. If it's cached, or just a string value, then that's fine. But if they're looking it up from EC2 metadata, then this would hit the endpoint twice. Probably fine, but potentially a performance/side-effect issue.
Technically we can delay the traced call the way the OTel middleware does, but it's gross and requires relying on orchestrion's mutable result swapping. I think the approach here is better, tbh, because it is more aligned with the paved-path of "define a tracing channel and just use it" rather than using orchestrion as a backdoor to monkeypatching. But if this becomes a problem in the future, that could be a way to address it.
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.
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.
Co-authored-by: Jan Peer Stöcklmair <jan.peer@sentry.io>
…/utils.ts Co-authored-by: Jan Peer Stöcklmair <jan.peer@sentry.io>
Co-authored-by: Jan Peer Stöcklmair <jan.peer@sentry.io>
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
from
cdb3108 to
1f13ab5
Compare
3 participants
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