-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
size-limit report 📦
|
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.
|
b361f67
to
8180ea7
Compare
packages/node-core/src/integrations/http/httpServerIntegration.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
8180ea7
to
a2bf73c
Compare
packages/node-core/src/integrations/http/httpServerIntegration.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
packages/node-core/src/integrations/http/httpServerIntegration.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
packages/node-core/src/integrations/http/httpServerSpansIntegration.ts
Outdated
Show resolved
Hide resolved
a8df443
to
cd42f95
Compare
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.
There was a problem hiding this 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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
bc64de5
to
7162c76
Compare
7162c76
to
3154341
Compare
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 functionalityhttpServerSpansIntegration
- emitshttp.server
spans for incoming requestsThe
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 thehttpIntegration
). The reason is to remain backwards compatible with users using/customizing thehttpIntegration
. 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
oroutgoingXXX
). 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 thehttpServerSpansIntegration
can register a callback for itself and plug into this with little overhead.