Uh oh!
There was an error while loading. Please reload this page.
feat(aws-serverless): Use orchestrion aws-sdk integration under diagnostics-channel opt-in - #22143
Conversation
Uh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
bdc838e to
9297436Compare9297436 to
31816fdCompare0b89e37 to
a3978faCompareUh oh!
There was an error while loading. Please reload this page.
a3978fa to
809da29Compare809da29 to
8907ff9Compare8907ff9 to
c0e10d6Comparec0e10d6 to
561e41bCompare561e41b to
8ffb46bCompare8ffb46b to
556da6fCompare556da6f to
9f3cab7Compare| // its channel-based equivalent AND the full channel-integration set is appended (mysql, postgres, | ||
| // express, ...), giving opted-in apps performance coverage this SDK's defaults otherwise omit. | ||
| // No-op otherwise. | ||
| return applyDiagnosticsChannelInjectionIntegrations(integrations, options); |
There was a problem hiding this comment.
This goes a bit against the default path where we don't load performance integrations, but since orchestrion is an opt-in feature this seems fine for now.
We will re-evaluate when we work on v11.
There was a problem hiding this comment.
what do you mean by this? the integrations that we load are the same before and after iiuc
There was a problem hiding this comment.
On develop we currently only include integrations without performance + aws lambda + aws sdk integrations. Here we straight up just include everything.
This is fine for the transition period but once we switch this over we should configure it the same way to not actually include everything out of the box, i.e. we should follow the same path as is currently. This is a bit cumbersome to do on v10 but shouldn't be a problem in v11.
209ce0f to
58a581dComparebatman begin |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 58a581d. Configure here.
d00cea8 to
aa8192cCompare19a3e72 to
aa8192cComparef3c38b1 to
aa8192cCompareUh oh!
There was an error while loading. Please reload this page.
| spanStreamingIntegration, | ||
| withStreamedSpan, | ||
| experimentalUseDiagnosticsChannelInjection, | ||
| diagnosticsChannelInjectionIntegrations, |
There was a problem hiding this comment.
q: why do we need to export this? like what use case does this cover
There was a problem hiding this comment.
This is needed to handle the different inits that the aws-serverless sdk has (i.e. npm aws init and aws layer init). It can be removed again in v11, it's just an awkward workaround right now. Similar to the other comment I left on this PR around init.
| // its channel-based equivalent AND the full channel-integration set is appended (mysql, postgres, | ||
| // express, ...), giving opted-in apps performance coverage this SDK's defaults otherwise omit. | ||
| // No-op otherwise. | ||
| return applyDiagnosticsChannelInjectionIntegrations(integrations, options); |
There was a problem hiding this comment.
what do you mean by this? the integrations that we load are the same before and after iiuc
| * produce nothing but spans, so this is gated on span recording. Exported so SDKs that build their | ||
| * own default-integration set can apply the same logic instead of duplicating it. | ||
| */ | ||
| export function applyDiagnosticsChannelInjectionIntegrations( |
There was a problem hiding this comment.
l: we have very similar logic in the node init, should we refactor this to reuse this function?
There was a problem hiding this comment.
This basically disappears in v11, I think it's not worth extracting it now.
Uh oh!
There was an error while loading. Please reload this page.
…ostics-channel opt-in
Wires the orchestrion aws-sdk channel integration into
@sentry/aws-serverless, where the OTelAwsintegration ships as a default today.@sentry/nodeexposes a reusableapplyDiagnosticsChannelInjectionIntegrationshelper (extracted from itsgetDefaultIntegrations), and@sentry/aws-serverlessuses it to swap the OTelAwsintegration for the channel version when the app opts in viaexperimentalUseDiagnosticsChannelInjection()(now re-exported from the aws-serverless SDK). No change when the opt-in isn't used.The existing
aws-integrationnode-integration-tests assert the swapped origin viaisOrchestrionEnabled(), so both the OTel and diagnostics-channel paths are covered by the same suites across both smithy stacks (latest + legacy3.1041.0) and ESM/CJS. Only the expected origin is parametrized.Part of #20946