Uh oh!
There was an error while loading. Please reload this page.
feat(server-utils): Add orchestrion aws-sdk channel integration core - #22142
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
6b85478 to
ca51438CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
61916ca to
828e59fCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| }, | ||
| { | ||
| "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
exportclassServicesExtensionsimplementsServiceExtension{
#services =newMap<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', |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
88e502d to
cdb3108Compare
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.
1f13ab5 to
95b65ddCompareUh oh!
There was an error while loading. Please reload this page.
83ad645 to
8889659CompareThere 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 8889659. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
75e28c4 to
54789f2CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
54789f2 to
298d80eCompare
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