fix(node): Avoid double-wrapping http module #16177
Conversation
| @@ -451,6 +369,17 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): | |||
| } | |||
| } | |||
|
|
|||
| function getRequestOptions(request: http.ClientRequest): http.RequestOptions { | |||
There was a problem hiding this comment.
this is technically kind-of breaking as we do not have all the possible fields here, but IMHO it should be fine and we have the most relevant things here. We only use this as second argument passed to ignoreOutgoingRequests(), for which this should be fine I'd say - the type does not change, at the very least 😅
There was a problem hiding this comment.
Is this something we have to call out when releasing?
There was a problem hiding this comment.
probably does not hurt, I will actually add a section to changelog right away!
|
Follow-up tasks: Can we also replace the Another follow-up task: This actually makes #15735 much easier to implement, because we can use the |
size-limit report 📦
|
mydea
force-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
72ddee3 to
596dcee
Compare
| @@ -62,7 +62,6 @@ test.describe('tracing in dynamically rendered (ssr) routes', () => { | |||
| }); | |||
|
|
|||
| expect(serverPageRequestTxn).toMatchObject({ | |||
| breadcrumbs: expect.any(Array), | |||
There was a problem hiding this comment.
this used to be the breadcrumbs of other errors from other tests.. which seems wrong to me, actually? So I wonder if this actually fixes something with astro, which is possible as now this is unrelated to when this is initialized, I guess..
There was a problem hiding this comment.
I guess this could fix some kind of isolation problem since request isolation in Astro was limited to the middleware lifecycle previously. Maybe something didn't work as expected in there or there was a timing issue. Gonna say it's finde until proven otherwise :D
packages/node/src/integrations/http/SentryHttpInstrumentation.ts
Outdated
Show resolved
Hide resolved
| @@ -451,6 +369,17 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): | |||
| } | |||
| } | |||
|
|
|||
| function getRequestOptions(request: http.ClientRequest): http.RequestOptions { | |||
There was a problem hiding this comment.
Is this something we have to call out when releasing?
| @@ -62,7 +62,6 @@ test.describe('tracing in dynamically rendered (ssr) routes', () => { | |||
| }); | |||
|
|
|||
| expect(serverPageRequestTxn).toMatchObject({ | |||
| breadcrumbs: expect.any(Array), | |||
There was a problem hiding this comment.
I guess this could fix some kind of isolation problem since request isolation in Astro was limited to the middleware lifecycle previously. Maybe something didn't work as expected in there or there was a timing issue. Gonna say it's finde until proven otherwise :D
Co-authored-by: Andrei <[email protected]>
mydea
force-pushed
the
fn/http-instrumentation-diagnostics-channel
branch
from
aff551f to
0cd1834
Compare
| In order to solve this problem, the modules are no longer monkey patched by us for request isolation. Instead, we register diagnostics*channel hooks to handle request isolation now. | ||
| While this is generally not expected to break anything, there is one tiny change that \_may* affect you if you have been relying on very specific functionality: |
There was a problem hiding this comment.
l: Markdown formatting here seems odd, is that on purpose?
There was a problem hiding this comment.
I tested this in a test application which showed double root spans and this fixed the problem!
However, as I was testing the routes in the browser it was requesting favicon.ico (which was not defined) and therefore showed a GET / transaction with the http.target: /favicon.ico. As we talked about in person, we should in general filter out 4xx spans (related issue #11720).
The default route name is always / in our express 5 instrumentation (below) and in the OTel instrumentation
@s1gr1d and @Lms24 figured out that the duplicate spans we sometimes see are related to esm and the http module. Especially, it seems to be related to us using the
stealthWrapfunction to wrapserver.emitfor request isolation purposes.While we still don't really know why this is making such problems, this PR seems to fix it (at least in integration tests) by avoiding using import-in-the-middle here, and instead using diagnostics channel with good old-fashioned monkey patching on the passed-in
serverinstance.Some note: We need to make sure to still call this in the otel-wrapping code of
init(), otherwise there are weird timing issues in top-level-import scenarios 😬Hopefully fixes #15830, and fixes #15803