Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(node): Split up http integration into composable parts #17524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mydea wants to merge 11 commits into develop
base: develop
Choose a base branch
Loading
from fn/extract-http-server-intergation

Conversation

Copy link
Member

@mydea mydea commented Sep 3, 2025

This is a first step to de-compose the node/node-core httpIntegration into multiple composable parts:

  • httpServerIntegration - handles request isolation, sessions, trace continuation, so core Sentry functionality
  • httpServerSpansIntegration - emits http.server spans for incoming requests

The httpIntegration sets these up under the hood, and for now it is not really recommended for users to use these stand-alone (though it is possible if users opt-out of the httpIntegration). The reason is to remain backwards compatible with users using/customizing the httpIntegration. We can revisit this in a later major.

These new integrations have a much slimmer API surface, and also allows us to avoid having to prefix all the options etc. with what they are about (e.g. incomingXXX or outgoingXXX). It also means you can actually tree-shake certain features (span creation) out, in theory.

Outgoing request handling remains the same for the time being, once we decoupled this from the otel http instrumentation we can do something similar there.

The biggest challenge was how to make it possible to de-compose this without having to monkey patch the http server twice. I opted to allow to add callbacks to the httpServerIntegration which it will call on any request. So the httpServerSpansIntegration can register a callback for itself and plug into this with little overhead.

@mydea mydea self-assigned this Sep 3, 2025
Copy link
Contributor

github-actions bot commented Sep 3, 2025
edited
Loading

size-limit report 📦

Path Size % Change Change
@sentry/browser 24.23 kB - -
@sentry/browser - with treeshaking flags 22.74 kB - -
@sentry/browser (incl. Tracing) 40.34 kB - -
@sentry/browser (incl. Tracing, Replay) 78.72 kB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 68.38 kB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 83.39 kB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 95.59 kB - -
@sentry/browser (incl. Feedback) 40.95 kB - -
@sentry/browser (incl. sendFeedback) 28.88 kB - -
@sentry/browser (incl. FeedbackAsync) 33.8 kB - -
@sentry/react 25.94 kB - -
@sentry/react (incl. Tracing) 42.32 kB - -
@sentry/vue 28.72 kB - -
@sentry/vue (incl. Tracing) 42.14 kB - -
@sentry/svelte 24.25 kB - -
CDN Bundle 25.74 kB - -
CDN Bundle (incl. Tracing) 40.16 kB - -
CDN Bundle (incl. Tracing, Replay) 76.38 kB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 81.88 kB - -
CDN Bundle - uncompressed 75.23 kB - -
CDN Bundle (incl. Tracing) - uncompressed 118.89 kB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 234.01 kB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 246.78 kB - -
@sentry/nextjs (client) 44.34 kB - -
@sentry/sveltekit (client) 40.76 kB - -
@sentry/node-core 50.32 kB +0.68% +337 B 🔺
@sentry/node 152.58 kB +0.26% +390 B 🔺
@sentry/node - without tracing 92.13 kB +0.28% +250 B 🔺
@sentry/aws-serverless 105.7 kB +0.36% +369 B 🔺

View base workflow run

Copy link
Contributor

github-actions bot commented Sep 3, 2025
edited
Loading

node-overhead report 🧳

Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.

Scenario Requests/s % of Baseline Prev. Requests/s Change %
GET Baseline 8,429 - 8,789 -4%
GET With Sentry 1,290 15% 1,360 -5%
GET With Sentry (error only) 5,842 69% 5,924 -1%
POST Baseline 1,154 - 1,201 -4%
POST With Sentry 474 41% 516 -8%
POST With Sentry (error only) 1,005 87% 1,035 -3%
MYSQL Baseline 3,194 - 3,265 -2%
MYSQL With Sentry 367 11% 433 -15%
MYSQL With Sentry (error only) 2,581 81% 2,673 -3%

View base workflow run

@mydea mydea force-pushed the fn/extract-http-server-intergation branch 3 times, most recently from b361f67 to 8180ea7 Compare September 8, 2025 09:03
* This integration handles request isolation, trace continuation and other core Sentry functionality around incoming http requests
* handled via the node `http` module.
*/
export const httpServerIntegration = _httpServerIntegration as (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exporting the types that we depend on here directly so we can call this in a type-safe way.

Lms24 reacted with thumbs up emoji
@mydea mydea marked this pull request as ready for review September 8, 2025 14:14
@mydea mydea force-pushed the fn/extract-http-server-intergation branch from 8180ea7 to a2bf73c Compare September 8, 2025 14:14
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for splitting this up! I think this makes the integrations more readable.

/**
* Register a client callback that will be called when a request is received.
*/
export function registerServerCallback(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the callback idea is fine for now but just to confirm: There's no scenario in which users would only register e.g. httpSeverSpansIntegration but NOT httpServerIntegration, right? In that case it wouldn't work I suppose but this is fine if it's out of scope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll log a warning in this case for now, IMHO that makes sense. I don't think we need to handle this as of now, we can say the one requires the other :)

@mydea mydea force-pushed the fn/extract-http-server-intergation branch 3 times, most recently from a8df443 to cd42f95 Compare September 18, 2025 07:45
Copy link
Member Author

mydea commented Sep 18, 2025

FYI I rewrote this to use a hook & non-enumerable property on the request to handle the separation of concerns, vs. keeping this in module scope per-client. I think this is the safer option to go as it means we should be resilient against module scope issues, as well as being more performant because we literally only expect/allow a single callback, not making this generic.

s1gr1d reacted with thumbs up emoji

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the client hook refactor is a good idea!


const flushPendingClientAggregates = (): void => {
clearTimeout(timeout);
unregisterClientFlushHook();
Copy link
Member

@Lms24 Lms24 Sep 19, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we're listening to more than one 'flush' event but as I learned the hard way inhttps://github.com//pull/17272, synchronously unsubscribing from the hook in the same callback is dangerous. It shouldn't be though, so I'll take a look at #17276 to fix this. If we wanna be extra sure to not cause array mutations here, we can use something like safeUnsubscribe in #16893. Otherwise, we can also ignore this for now and wait for the general fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, interesting! This has not changed here (just moved around) but sounds reasonable to me just fix this generally with your other PR 👍

@mydea mydea force-pushed the fn/extract-http-server-intergation branch from bc64de5 to 7162c76 Compare September 19, 2025 12:04
@mydea mydea force-pushed the fn/extract-http-server-intergation branch from 7162c76 to 3154341 Compare September 22, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seer-by-sentry seer-by-sentry[bot] seer-by-sentry[bot] left review comments

@Lms24 Lms24 Lms24 approved these changes

@s1gr1d s1gr1d s1gr1d approved these changes

@andreiborza andreiborza Awaiting requested review from andreiborza

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /