Uh oh!
There was an error while loading. Please reload this page.
feat(node): Always set up express, fastify, koa, hapi integrations - #23473
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.
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
d405a64 to
8a9d8c4CompareUh oh!
There was an error while loading. Please reload this page.
7c5156c to
dee852fCompareUh oh!
There was an error while loading. Please reload this page.
3e1ee6e to
20ff2d2Compare
isaacs
left a comment
There was a problem hiding this comment.
This is a good fix to the main issue raised in the fastify PR.
Only a few small suggestions in the code here, but the bigger issue is that even with this, 3 of the 4 mentioned integrations actually still don't do anything if tracing is disabled, because they gate on getActiveSpan() !== undefined.
That could of course be done in a followup, but we may as well do it now. Here's a patch that would close the loop on it: https://gist.github.com/isaacs/c79cb5d86b5c63431a2442dbbf0ae2ee
There was a problem hiding this comment.
Not changed in this PR, but I think we still want this detection to happen, especially now that we're relying on it for error handling and transaction names. The docs for the option say that you should set it false if you have orchestrion in the bundle, but the detection is for either bundle or runtime injection, so it's valuable to keep.
| detectOrchestrionSetup(); |
Uh oh!
There was an error while loading. Please reload this page.
| // Framework-level integrations | ||
| expressIntegration(), | ||
| fastifyIntegration(), | ||
| hapiIntegration(), | ||
| koaIntegration(), |
There was a problem hiding this comment.
These four show up in a few places now:
- here (packages/node/src/sdk/index.ts)
- packages/bun/src/sdk.ts
- packages/deno/src/sdk.ts
I fee like this is prone to overlooking, if we add another framework that belongs to this set. Could it live in a list somewhere centrally reusable?
There was a problem hiding this comment.
moved this to a helper exported from server-utils which all of these use!
| }; | ||
| } | ||
| export interface ErrorSink { |
There was a problem hiding this comment.
This looks like a copy-paste of TransactionSink above it. Could we abstract them both in to a function makeSink<T>(): { record(e: T): null; waitFor(p): Promise<T> } reusable?
It's a test, so not super high priority, but could be a bit less to look at.
| // The following integrations are not considered performance integrations because they are "framework"-level | ||
| // meaning they may also handle error capture and similar things. | ||
| // Thus, we add them by default: | ||
| // express, fastify, hapi, koa |
There was a problem hiding this comment.
I feel like this comment is weirdly placed. Shouldn't it be at packages/node/src/sdk/index.ts line 73, or wherever the integrations ultimately get moved to?
There was a problem hiding this comment.
hmm the idea was to make it clear right here why these are not in this list. but maybe it is unnecessary and we just remove the comment overall, it should be possible to figure this out looking at the code of default integrations anyhow 😅
20ff2d2 to
7020156CompareUh 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.
Mirror the Node SDK change promoting the framework integrations (express, fastify, hapi, koa) to always-on defaults. Bun gains all four; Deno (which already listed express, hapi, koa) gains fastify, now also re-exported from `@sentry/server-utils/orchestrion`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror the other orchestrion Deno suites: assert the Fastify integration is in the defaults and that the native `tracing:fastify.request.handler:error` channel captures the error (with mechanism `auto.function.fastify`). Adds a shared `errorSink` helper alongside `transactionSink`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
081f193 to
ee17f3dCompareThere 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 ee17f3d. Configure here.
| prismaIntegration, | ||
| amqplibIntegration, | ||
| anthropicAIIntegration, | ||
| expressIntegration, |
There was a problem hiding this comment.
Public helper drops framework integrations
Medium Severity
getAutoPerformanceIntegrations is still a public @sentry/node export, but it now only returns getTracingIntegrations(), so callers no longer receive express, fastify, hapi, or koa. Those moved to getErrorIntegrations, which @sentry/node does not re-export, so there is no documented replacement for the previous full set. This is a public API behavior change without a deprecation notice.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit ee17f3d. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
) Stacked on #23473. Adds a build-time opt-out for the Node SDK's runtime diagnostics-channel injection, complementing the runtime `enableRuntimeChannelInjection` option from the base PR. - New `__SENTRY_CHANNEL_INJECTION__` treeshaking flag, mirroring `__SENTRY_TRACING__`: when a bundler text-replaces it with `false`, the `registerDiagnosticsChannelInjection()` / `detectOrchestrionSetup()` block in `init` is dropped, and its transitive orchestrion-register code tree-shakes away. - Exposed through the bundler plugins' `bundleSizeOptimizations.excludeChannelInjection`, which maps to `__SENTRY_CHANNEL_INJECTION__ = false` via the same mechanism as `excludeTracing`. - The gate now reads both the build flag and the runtime option: `(typeof __SENTRY_CHANNEL_INJECTION__ === 'undefined' || __SENTRY_CHANNEL_INJECTION__) && options.enableRuntimeChannelInjection !== false`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: isaacs <i@izs.me>
These integrations may also do non-tracing related things (e.g. error capturing etc), so they should be added by default.
This also means that we always want to add the orchestrion runtime, even in non-tracing mode.
Instead, I added a flag
enableRuntimeChannelInjectionto opt-out of this behavior if needed.Also added these three as defaults to deno and bun, to mirror this.
Additionally, this unifies the integrations we add into
getErrorIntegrations()andgetTracingIntegrations(), so we can re-use this in node, deno and bun. It also adds awsIntegration to the tracing set so this is also added in all scenarios now.