feat(http2): trace core-API HTTP/2 servers#9138
Conversation
A server using the raw stream API (`createServer().on('stream', ...)`) produced
no server span, because the instrumentation only hooked the compatibility
`request` event. This adapts the `Http2Stream` and its pseudo-header map into the
minimal req/res shape the shared web lifecycle consumes, so the core path now
gets the same span, context propagation, and header tagging as the compatibility
path.
A compatibility server emits both `request` and `stream` for every request, so
the `stream` branch only creates a span when the server has no `request`
listener; otherwise the request would be double-instrumented.
Fixes: #312
Overall package sizeSelf size: 6.72 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.1 | 122.62 kB | 437.94 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2026-07-10 17:50:37 Comparing candidate commit 4f6c8d3 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2248 metrics, 38 unstable metrics.
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9138 +/- ##
==========================================
+ Coverage 93.69% 96.58% +2.88%
==========================================
Files 889 918 +29
Lines 50864 121759 +70895
Branches 11834 20709 +8875
==========================================
+ Hits 47657 117599 +69942
- Misses 3207 4160 +953
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A core-API stream aborted before `stream.respond()` runs (client RST, server `stream.close(code)`, a throwing handler) has an empty `stream.sentHeaders`, so the adapter reported `res.statusCode` as `undefined`. `web.js` then ran `validateStatus(undefined)`, which is falsy, tagged the span as an error, and dropped the `http.status_code` tag — diverging from the compatibility path, whose `Http2ServerResponse.statusCode` defaults to 200 for the same abort. The adapter now falls back to 200 so both paths agree. Drive-by fix: * Pin the stream-adapter req/res shape with a `@typedef` naming the `web.js` / `url.js` / `ip_extractor.js` fields it must satisfy, so a new read added there fails the type check instead of silently resolving to `undefined` on the core path only. * Record at `bindEmit` that finish is single-sourced from the stream's one `close` event, which is why the `!req.stream` idempotency-guard bypass in `web.js` is harmless here. Fixes: #312
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac52b7ff9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A server that registers both a raw `stream` listener and a compatibility `request` listener emits a single `stream` event whose internal listener synthesizes the `request`. The previous gate created the span only on the synthesized `request`, which fires nested in one stream listener, so the application's own `stream` listener ran with no active span and any child spans or handler duration it produced were lost. 1. Such mixed servers now create the span from the `stream` event, keeping it active across the application's stream listener; the synthesized `request` reuses it instead of creating a second span. A request-only server still traces on `request` so the compatibility response keeps its richer req/res, and the per-request cost stays a single `WeakSet.has`. 2. The `with configured headers` test reconfigured the plugin with a second `agent.load` nested under the suite that already loaded it, starting a second mock agent and leaking the first server's handle; it now reloads in place so a single agent runs. Drive-by fix: * Correct the stream-adapter `@typedef`: `res.statusCode` is the numeric `:status` pseudo-header, and `res.getHeader` can return a string array.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aebbb6938a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…channel A server with both a raw `stream` listener and a compatibility `request` listener creates its span from the `stream` event using a throwaway adapter. The synthesized `request` off the same stream then found no context, so a user's `request` handler calling `web.setRoute`/`web.setFramework` never reached the span and the finish `hooks.request` received the adapter instead of the real `Http2ServerRequest`/`Http2ServerResponse`. The instrumentation now publishes `apm:http2:server:request:adopt` with the real req/res, and the plugin points the stream-backed context at them. The join key is the shared `Http2Stream`: `web.linkContextToStream` keys the context on the stream so the second request resolves to the first's span. That key is written only for mixed servers (`ctx.adoptable`), so the common single-listener request pays no extra per-request map write.
@grpc/grpc-js builds its transport on a core Node `Http2Server` and registers a raw `stream` listener with no `request` listener. The re-enabled http2 server instrumentation traces exactly that shape, so every gRPC call gained a `web.request` span on top of its `grpc.server` span, and because the stream event fires before gRPC's handler, `web.request` became the top frame. The gRPC instrumentation now marks the server it owns and the http2 instrumentation skips any marked server, so a gRPC call keeps a single span with gRPC as the top frame. Suppression is unconditional: disabling gRPC tracing does not resurrect these as `web.request` spans, since a gRPC call over HTTP/2 has no meaningful HTTP request/response semantics (`:path` is `/pkg.Svc/Method`, status is always 200 + trailers) and surfacing it as one would surprise a user who turned gRPC off. The mark is a module-local `Symbol` on the server instance, read once per emit behind the existing `hasSubscribers` gate (~1.6 ns; a WeakSet lookup measured ~3.5x slower and needs a shared mutable container across the two instrumentations). It is set from `_setupHandlers`, the single server-creation funnel present across the supported range — the newer `createHttp2Server` does not exist on older versions — and it runs at bind time, before any request reaches the wrapped `emit`.
Linux runners resolve localhost to ::1, but the gRPC fixture listens on IPv4, so the probe failed before reaching the server.
A synchronous throw from a core stream handler terminates the process, so exercise it in a forked fixture and let NYC collect the child coverage instead of excluding the error path.
* feat(http2): trace core-API HTTP/2 servers
A server using the raw stream API (`createServer().on('stream', ...)`) produced
no server span, because the instrumentation only hooked the compatibility
`request` event. This adapts the `Http2Stream` and its pseudo-header map into the
minimal req/res shape the shared web lifecycle consumes, so the core path now
gets the same span, context propagation, and header tagging as the compatibility
path.
A compatibility server emits both `request` and `stream` for every request, so
the `stream` branch only creates a span when the server has no `request`
listener; otherwise the request would be double-instrumented.
Fixes: #312
* fix(http2): default core-stream status to 200 when no response was sent
A core-API stream aborted before `stream.respond()` runs (client RST, server
`stream.close(code)`, a throwing handler) has an empty `stream.sentHeaders`, so
the adapter reported `res.statusCode` as `undefined`. `web.js` then ran
`validateStatus(undefined)`, which is falsy, tagged the span as an error, and
dropped the `http.status_code` tag — diverging from the compatibility path,
whose `Http2ServerResponse.statusCode` defaults to 200 for the same abort. The
adapter now falls back to 200 so both paths agree.
Drive-by fix:
* Pin the stream-adapter req/res shape with a `@typedef` naming the `web.js` /
`url.js` / `ip_extractor.js` fields it must satisfy, so a new read added there
fails the type check instead of silently resolving to `undefined` on the core
path only.
* Record at `bindEmit` that finish is single-sourced from the stream's one
`close` event, which is why the `!req.stream` idempotency-guard bypass in
`web.js` is harmless here.
Fixes: #312
* fix(http2): keep the server span active for mixed stream/request servers
A server that registers both a raw `stream` listener and a compatibility
`request` listener emits a single `stream` event whose internal listener
synthesizes the `request`. The previous gate created the span only on the
synthesized `request`, which fires nested in one stream listener, so the
application's own `stream` listener ran with no active span and any child
spans or handler duration it produced were lost.
1. Such mixed servers now create the span from the `stream` event, keeping
it active across the application's stream listener; the synthesized
`request` reuses it instead of creating a second span. A request-only
server still traces on `request` so the compatibility response keeps its
richer req/res, and the per-request cost stays a single `WeakSet.has`.
2. The `with configured headers` test reconfigured the plugin with a second
`agent.load` nested under the suite that already loaded it, starting a
second mock agent and leaking the first server's handle; it now reloads
in place so a single agent runs.
Drive-by fix:
* Correct the stream-adapter `@typedef`: `res.statusCode` is the numeric
`:status` pseudo-header, and `res.getHeader` can return a string array.
* fix(http2): hand mixed-server requests the real req/res via an adopt channel
A server with both a raw `stream` listener and a compatibility `request`
listener creates its span from the `stream` event using a throwaway adapter.
The synthesized `request` off the same stream then found no context, so a
user's `request` handler calling `web.setRoute`/`web.setFramework` never
reached the span and the finish `hooks.request` received the adapter instead
of the real `Http2ServerRequest`/`Http2ServerResponse`.
The instrumentation now publishes `apm:http2:server:request:adopt` with the
real req/res, and the plugin points the stream-backed context at them. The
join key is the shared `Http2Stream`: `web.linkContextToStream` keys the
context on the stream so the second request resolves to the first's span. That
key is written only for mixed servers (`ctx.adoptable`), so the common
single-listener request pays no extra per-request map write.
* fix(http2): do not trace gRPC-owned HTTP/2 servers
@grpc/grpc-js builds its transport on a core Node `Http2Server` and registers
a raw `stream` listener with no `request` listener. The re-enabled http2
server instrumentation traces exactly that shape, so every gRPC call gained a
`web.request` span on top of its `grpc.server` span, and because the stream
event fires before gRPC's handler, `web.request` became the top frame.
The gRPC instrumentation now marks the server it owns and the http2
instrumentation skips any marked server, so a gRPC call keeps a single span
with gRPC as the top frame. Suppression is unconditional: disabling gRPC
tracing does not resurrect these as `web.request` spans, since a gRPC call over
HTTP/2 has no meaningful HTTP request/response semantics (`:path` is
`/pkg.Svc/Method`, status is always 200 + trailers) and surfacing it as one
would surprise a user who turned gRPC off.
The mark is a module-local `Symbol` on the server instance, read once per emit
behind the existing `hasSubscribers` gate (~1.6 ns; a WeakSet lookup measured
~3.5x slower and needs a shared mutable container across the two
instrumentations). It is set from `_setupHandlers`, the single server-creation
funnel present across the supported range — the newer `createHttp2Server` does
not exist on older versions — and it runs at bind time, before any request
reaches the wrapped `emit`.
* test(grpc): connect non-gRPC probe over IPv4
Linux runners resolve localhost to ::1, but the gRPC fixture listens on IPv4, so the probe failed before reaching the server.
* test(http2): cover stream handler crash path
A synchronous throw from a core stream handler terminates the process, so exercise it in a forked fixture and let NYC collect the child coverage instead of excluding the error path.
* feat(http2): trace core-API HTTP/2 servers
A server using the raw stream API (`createServer().on('stream', ...)`) produced
no server span, because the instrumentation only hooked the compatibility
`request` event. This adapts the `Http2Stream` and its pseudo-header map into the
minimal req/res shape the shared web lifecycle consumes, so the core path now
gets the same span, context propagation, and header tagging as the compatibility
path.
A compatibility server emits both `request` and `stream` for every request, so
the `stream` branch only creates a span when the server has no `request`
listener; otherwise the request would be double-instrumented.
Fixes: #312
* fix(http2): default core-stream status to 200 when no response was sent
A core-API stream aborted before `stream.respond()` runs (client RST, server
`stream.close(code)`, a throwing handler) has an empty `stream.sentHeaders`, so
the adapter reported `res.statusCode` as `undefined`. `web.js` then ran
`validateStatus(undefined)`, which is falsy, tagged the span as an error, and
dropped the `http.status_code` tag — diverging from the compatibility path,
whose `Http2ServerResponse.statusCode` defaults to 200 for the same abort. The
adapter now falls back to 200 so both paths agree.
Drive-by fix:
* Pin the stream-adapter req/res shape with a `@typedef` naming the `web.js` /
`url.js` / `ip_extractor.js` fields it must satisfy, so a new read added there
fails the type check instead of silently resolving to `undefined` on the core
path only.
* Record at `bindEmit` that finish is single-sourced from the stream's one
`close` event, which is why the `!req.stream` idempotency-guard bypass in
`web.js` is harmless here.
Fixes: #312
* fix(http2): keep the server span active for mixed stream/request servers
A server that registers both a raw `stream` listener and a compatibility
`request` listener emits a single `stream` event whose internal listener
synthesizes the `request`. The previous gate created the span only on the
synthesized `request`, which fires nested in one stream listener, so the
application's own `stream` listener ran with no active span and any child
spans or handler duration it produced were lost.
1. Such mixed servers now create the span from the `stream` event, keeping
it active across the application's stream listener; the synthesized
`request` reuses it instead of creating a second span. A request-only
server still traces on `request` so the compatibility response keeps its
richer req/res, and the per-request cost stays a single `WeakSet.has`.
2. The `with configured headers` test reconfigured the plugin with a second
`agent.load` nested under the suite that already loaded it, starting a
second mock agent and leaking the first server's handle; it now reloads
in place so a single agent runs.
Drive-by fix:
* Correct the stream-adapter `@typedef`: `res.statusCode` is the numeric
`:status` pseudo-header, and `res.getHeader` can return a string array.
* fix(http2): hand mixed-server requests the real req/res via an adopt channel
A server with both a raw `stream` listener and a compatibility `request`
listener creates its span from the `stream` event using a throwaway adapter.
The synthesized `request` off the same stream then found no context, so a
user's `request` handler calling `web.setRoute`/`web.setFramework` never
reached the span and the finish `hooks.request` received the adapter instead
of the real `Http2ServerRequest`/`Http2ServerResponse`.
The instrumentation now publishes `apm:http2:server:request:adopt` with the
real req/res, and the plugin points the stream-backed context at them. The
join key is the shared `Http2Stream`: `web.linkContextToStream` keys the
context on the stream so the second request resolves to the first's span. That
key is written only for mixed servers (`ctx.adoptable`), so the common
single-listener request pays no extra per-request map write.
* fix(http2): do not trace gRPC-owned HTTP/2 servers
@grpc/grpc-js builds its transport on a core Node `Http2Server` and registers
a raw `stream` listener with no `request` listener. The re-enabled http2
server instrumentation traces exactly that shape, so every gRPC call gained a
`web.request` span on top of its `grpc.server` span, and because the stream
event fires before gRPC's handler, `web.request` became the top frame.
The gRPC instrumentation now marks the server it owns and the http2
instrumentation skips any marked server, so a gRPC call keeps a single span
with gRPC as the top frame. Suppression is unconditional: disabling gRPC
tracing does not resurrect these as `web.request` spans, since a gRPC call over
HTTP/2 has no meaningful HTTP request/response semantics (`:path` is
`/pkg.Svc/Method`, status is always 200 + trailers) and surfacing it as one
would surprise a user who turned gRPC off.
The mark is a module-local `Symbol` on the server instance, read once per emit
behind the existing `hasSubscribers` gate (~1.6 ns; a WeakSet lookup measured
~3.5x slower and needs a shared mutable container across the two
instrumentations). It is set from `_setupHandlers`, the single server-creation
funnel present across the supported range — the newer `createHttp2Server` does
not exist on older versions — and it runs at bind time, before any request
reaches the wrapped `emit`.
* test(grpc): connect non-gRPC probe over IPv4
Linux runners resolve localhost to ::1, but the gRPC fixture listens on IPv4, so the probe failed before reaching the server.
* test(http2): cover stream handler crash path
A synchronous throw from a core stream handler terminates the process, so exercise it in a forked fixture and let NYC collect the child coverage instead of excluding the error path.
Summary
A server using the raw stream API (
createServer().on('stream', ...)) produced no server span, because the instrumentation only hooked the compatibilityrequestevent. Thestreamevent is now adapted into the minimal req/res shape the shared web lifecycle consumes, so the core path gets the same span, context propagation, and configured header tagging as the compatibility path.A compatibility server emits both
requestandstreamfor every request, so thestreambranch only creates a span when the server has norequestlistener; otherwise the request would be double-instrumented.Test plan
should instrument the core stream API— aweb.requestspan with the expected url/method/status for a core-API server.produces exactly one server spanpermutations —createServer(handler),createServer().on('request'), and a server with both listeners each yield exactly one span (the gate that prevents double-instrumentation).makes the core server span a child of the client span— distributed context extracted on the core path.tags configured request and response headers from the stream— header config reads through the stream adapter.Fixes: #312