feat(node)!: Make channel-based instrumentation the default#22501
feat(node)!: Make channel-based instrumentation the default#22501andreiborza wants to merge 41 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
921217b to
e9cde04
Compare
404fb86 to
c9a6733
Compare
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 11d7697. Configure here.
bd63462 to
3f9a563
Compare
9808b49 to
9bd4d80
Compare
Make orchestrion diagnostics-channel injection the default in `@sentry/node` and remove the `experimentalUseDiagnosticsChannelInjection()` opt-in. `Sentry.init()` now installs the channel-injection module hooks unconditionally when span recording is enabled, so the opt-in function, its loader indirection, and the name-based OTel->channel integration swap are gone. The self-selecting `dataloader` and `knex` integrations pick the channel path automatically now that `isOrchestrionInjected()` is true by default. Redis stays a composite integration: the native diagnostics-channel subscribers (node-redis >=5.12, ioredis >=5.11, batches) plus the orchestrion subscribers for the older ranges, all sharing the node cache `responseHook`. The vendored OTel patchers only run on runtimes without `tracingChannel` (Node <18.19). BREAKING CHANGE: `experimentalUseDiagnosticsChannelInjection()`, `diagnosticsChannelInjectionIntegrations()`, `isDiagnosticsChannelInjectionEnabled()` and `applyDiagnosticsChannelInjectionIntegrations()` are removed from `@sentry/node`. Channel-based instrumentation is now always on; no opt-in call is needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI-measured @sentry/node without tracing (97032B) edged just over the 97 KB limit; CI gzip runs slightly larger than local. Bump without-tracing to 98 KB and @sentry/node to 159 KB for margin.
getAutoPerformanceIntegrations now uses the channel integrations from @sentry/server-utils/orchestrion directly instead of routing through the thin @sentry/node wrappers. Redis and Fastify keep their node wrappers (cache responseHook wiring; Fastify v3/v4 coverage). The wrapper files and their public exports are unchanged; removing them and the vendored OTel is tracked in JS-3074.
9bd4d80 to
4dab6a2
Compare
d7aa19a to
50303d9
Compare
| "sentryTest": { | ||
| "skip": true | ||
| }, |
There was a problem hiding this comment.
/preload is going away so I'm just skipping here.
| "sentryTest": { | ||
| "skip": true | ||
| }, |
There was a problem hiding this comment.
/preload is going away so I'm just skipping here.
… tests" This reverts commit 506f090.
| "sentryTest": { | ||
| "skip": true | ||
| }, |
There was a problem hiding this comment.
Bring your own OTel will change and so the test apps need a bigger overhaul. Skipping for now.
| "sentryTest": { | ||
| "skip": true | ||
| }, |
There was a problem hiding this comment.
Bring your own OTel will change and so the test apps need a bigger overhaul. Skipping for now.
| export function isOrchestrionEnabled(): boolean { | ||
| return process.env.INJECT_ORCHESTRION === 'true' || process.env.INJECT_ORCHESTRION === '1'; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This will be removed in the follow-up where we remove the iitm instrumentations.
| export function isOrchestrionEnabled(): boolean { | ||
| return process.env.INJECT_ORCHESTRION === 'true' || process.env.INJECT_ORCHESTRION === '1'; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This will be removed in the follow-up where we remove the iitm instrumentations.
| // The streamlined `Fastify` integration covers fastify `>=3.21.0 <6`; `instrumentFastifyV3` | ||
| // fills the remaining early-v3 gap (`>=3.0.0 <3.21.0`), so it stays preloaded here. |
There was a problem hiding this comment.
This will be removed in the follow-up where we delete iitm instrumentations.
|
Looks like this will make the sveltekit-3 e2e stop failing. Not sure how it would be related to this PR though. https://gist.github.com/isaacs/fe3d9b27871a504f878f84f3bb454cd3 (EDIT: might've spoken too soon, still failing, just a different way. Anyway, unrelated to this.) I'll put up a PR to fix it once done reviewing this. |
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Some of these comments can definitely be postponed for a follow-up.
Worth noting that this is only removing the run time opt-in; several build-time opt-ins still remain, ie the _experimental.useDiagnosticsChannelInjection values:
$ git grep -l '\\.useDiagnosticsChannelInjection' -- packages/
packages/cloudflare/src/vite/index.ts
packages/nextjs/src/config/turbopack/constructTurbopackConfig.ts
packages/nextjs/src/config/types.ts
packages/nextjs/src/config/webpack.ts
packages/nextjs/src/config/withSentryConfig/buildTime.ts
packages/nextjs/src/config/withSentryConfig/getFinalConfigObject.ts
packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectBundlerUtils.ts
packages/nextjs/test/config/diagnosticsChannelInjection.test.ts
packages/nextjs/test/config/turbopack/constructTurbopackConfig.test.ts
packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts
packages/nuxt/src/common/types.ts
packages/nuxt/src/module.ts
packages/nuxt/test/vite/buildOptions.test-d.ts
Also, there are some stale references to the opt-in (in comments) in these files:
packages/nextjs/src/config/types.ts
packages/nuxt/src/common/types.ts
packages/server-utils/src/integrations/tracing-channel/express/index.ts
packages/server-utils/src/integrations/tracing-channel/kafkajs/index.ts
packages/server-utils/src/integrations/tracing-channel/postgres-js.ts
packages/server-utils/src/orchestrion/bundler/options.ts
packages/server-utils/src/orchestrion/index.ts
And there's still a lot of comments referencing "EXPERIMENTAL" or "opt-in" on the orchestrion tracing integrations that are now the default (only) option.
| // as possible, before the app imports its instrumented modules. | ||
| const useChannelInjection = hasSpansEnabled(optionsWithResolvedTracing); | ||
| if (useChannelInjection) { | ||
| registerDiagnosticsChannelInjection(); |
There was a problem hiding this comment.
I think this is getting called before the debug.log logger is enabled, right? If so, then the failure modes in registerDiagnosticsChannelInjection won't be logged.
If I'm reading that right, then it seems like we need to either move this down lower (where the detection happens) or somehow buffer the warning until the logger is available. It was less of an issue when OTel was the default, but now it's the only way to get tracing instrumentation, so the debug warning is more important.
| timestamp: expect.any(Number), | ||
| status: 'ok', | ||
| origin: 'auto.http.otel.nestjs', | ||
| origin: 'auto.http.orchestrion.nestjs', |
| @@ -172,7 +170,7 @@ export { | |||
| rewriteFramesIntegration, | |||
| } from '@sentry/core'; | |||
|
|
|||
| export { awsIntegration } from './integration/aws'; | |||
| export { awsChannelIntegration as awsIntegration } from '@sentry/server-utils/orchestrion'; | |||
There was a problem hiding this comment.
low/maybe out of scope: It'd be nice to name these just *Integration rather than *ChannelIntegration at the server-utils origin (and also update how they're referenced in Deno and Bun SDKs), since there's no other alternative anyway now.
| // the node cache options. The orchestrion channel integrations for the older ranges are appended | ||
| // separately (see `redisChannelIntegrations`). `cacheResponseHook` reads options set in the | ||
| // extension's `setupOnce` below, but it only runs at command time, by which point those options are set. | ||
| return extendIntegration(redisNativeChannelIntegration({ responseHook: cacheResponseHook }), { |
There was a problem hiding this comment.
I think this one dropped something.
Previously, this was combining three options: native-dc for new redis versions, plus the OTel/Orchestrion swap for the older versions. Now, the new native-dc is included here, but redisChannelIntegrations would have to be used for older redis versions, which isn't re-exported by the node SDK, but is included in the defaults.
So, if someone did defaultIntegrations: [], integrations: [ Sentry.redisIntegration(...) ], then they wouldn't get the orchestrion one.
I'd suggest folding the two orchestrion integrations into redisIntegration() so the public factory is the simple thing to use.
| // express, ...), giving opted-in apps performance coverage this SDK's defaults otherwise omit. | ||
| // No-op otherwise. | ||
| return applyDiagnosticsChannelInjectionIntegrations(integrations, options); | ||
| export function getDefaultIntegrations(_options: Options): Integration[] { |
There was a problem hiding this comment.
Agreed with your reply to clanker below (confirmed locally), but I think that means the _options param is unused and can be removed?
There was a problem hiding this comment.
and if so, then packages/aws-serverless/src/awslambda-auto.ts can drop the options it passes in, and has a dead comment:
// Sentry node's `getDefaultIntegrations` will load them if tracing is enabled,
// which is the case if `tracesSampleRate` is set.
| // which resolves the route on every layer. Without this, such transactions keep | ||
| // the raw URL name (e.g. `GET /trpc/foo` instead of `GET /trpc`). | ||
| const constructedRoute = getConstructedRoute(req); | ||
| const matchedRoute = constructedRoute != null ? getActualMatchedRoute(req, constructedRoute) : undefined; |
There was a problem hiding this comment.
This seems like an unrelated behavior change? Not opposed to it (if it's correct, it's correct, and may as well fix the bug) but surprised to see it in this. Is there a reason it was affected by making orchestrion the default?
| // recording: the channel integrations only produce spans, so with tracing off there are no | ||
| // subscribers and injecting the module hooks would be pointless work. Install the hooks as early | ||
| // as possible, before the app imports its instrumented modules. | ||
| const useChannelInjection = hasSpansEnabled(optionsWithResolvedTracing); |
There was a problem hiding this comment.
Just thinking through edge cases, if a user doesn't have tracing enabled, and then later switches it on, they won't get channels injected. Eg, someone could have a tracesSampler that returns 0 at this point, but then >0 later, and they won't get any injected channels.
Might be worth calling out in the docs or migration guide that tracing has to be set up at init time, or not at all? Technically this was also true of the OTel integrations, I guess, so it's not really a regression per se, but since they were able to monkey-patch at first require/import, it wasn't permanent.
| import { instrumentRedis, redisIntegration } from './redis'; | ||
| import { instrumentTedious, tediousIntegration } from './tedious'; | ||
| import { instrumentVercelAi, vercelAIIntegration } from './vercelai'; | ||
| import { redisChannelIntegrations } from './redis'; | ||
|
|
||
| /** |
|
Oh, also, there's a bunch of checks like this, for example in packages/server-utils/src/integrations/tracing-channel/koa.ts: // `tracingChannel` is unavailable before Node 18.19 so do nothing in that case.
if (!diagnosticsChannel.tracingChannel || subscribed) {
return;
}that can all be removed once this lands. They're harmless, but our new floor is 20.19.0, so presumably they do nothing. |
2 participants
What
Make orchestrion diagnostics-channel instrumentation the default in
@sentry/nodeand remove theexperimentalUseDiagnosticsChannelInjection()opt-in.Why
The channel-based instrumentations shipped opt-in in v10. Making them the default in v11 unlocks instrumentation at run and build time.
Removal of the vendored OTel/IITM code is intentionally out of scope; it is tracked separately as follow-up.
Closes: #22345