feat(node): Split up http integration into composable parts #17524
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
| * 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 ( |
There was a problem hiding this comment.
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.
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
| /** | ||
| * Register a client callback that will be called when a request is received. | ||
| */ | ||
| export function registerServerCallback( |
There was a problem hiding this comment.
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.
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.
I think the client hook refactor is a good idea!
|
|
||
| const flushPendingClientAggregates = (): void => { | ||
| clearTimeout(timeout); | ||
| unregisterClientFlushHook(); |
There was a problem hiding this comment.
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.
huh, interesting! This has not changed here (just moved around) but sounds reasonable to me just fix this generally with your other PR 👍
7162c76 to
3154341
Compare
398c5c0 to
f098c0b
Compare
6732caa to
be14b96
Compare
…y#17524) 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 incom 6854 ing 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.
This is a first step to de-compose the node/node-core
httpIntegrationinto multiple composable parts:httpServerIntegration- handles request isolation, sessions, trace continuation, so core Sentry functionalityhttpServerSpansIntegration- emitshttp.serverspans for incoming requestsThe
httpIntegrationsets 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.
incomingXXXoroutgoingXXX). 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
httpServerIntegrationwhich it will call on any request. So thehttpServerSpansIntegrationcan register a callback for itself and plug into this with little overhead.Note
Introduce
httpServerIntegrationandhttpServerSpansIntegration, refactor HTTP composition and add ahttpServerRequestclient hook; update exports and tests across SDKs.httpServerIntegration(request isolation, sessions, body capture) andhttpServerSpansIntegration(emithttp.serverspans; filtering, hooks).client.on/emit('httpServerRequest', request, response, normalizedRequest).httpIntegrationnow composeshttpServerIntegration+httpServerSpansIntegration+SentryHttpInstrumentation(outgoing breadcrumbs/trace headers only). Remove legacy incoming-requests path.WeakRef(tsconfig lib updated).httpServerIntegrationandhttpServerSpansIntegrationfrom@sentry/node-coreand re-export via@sentry/node,bun,aws-serverless,google-cloud-serverless,astro,remix,solidstart.transactionevents forhttp.serverspans with originauto.http.otel.http(e.g. prerendered pages).recordRequestSessionusage and promise buffer timeout expectation.Written by Cursor Bugbot for commit be14b96. This will update automatically on new commits. Configure here.