refactor(express): trace middleware via layer prototype dispatch#9067
Conversation
BenchmarksBenchmark execution time: 2026-07-07 20:57:52 Comparing candidate commit 6ea01fd in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2263 metrics, 20 unstable metrics.
|
Middleware tracing replaced `layer.handle` with a wrapper and re-exposed the user handler through an enumerable `_datadog_orig` back-reference, the only property loopback's `_findLayerByHandler` could follow to tag a layer with its phase. That ties the tracer to loopback's private reflection heuristic, where the property name and its enumerability are both load-bearing. Wrap the host `Layer` prototype dispatch (`handle_request`/`handle_error`, and `handleRequest`/`handleError` on router >=2) and read per-layer metadata from a WeakMap, leaving `layer.handle` the user's function. loopback's first lookup (`layer.handle === handler`) then matches with no tracer-specific contract, so the back-reference, the `express-async-errors` `__handle` branch, and the `_name` write-back onto the user handler are no longer needed. A synchronous throw or a rejected promise becomes `next(error)` in the host dispatch, so the wrapped `next` captures both without a tracer-side try/catch. Refs: #9062
bab9d0d to
5d5b152
Compare
Overall package sizeSelf size: 6.64 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 |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 6ea01fd | Docs | Datadog PR Page | Give us feedback! |
express <4.3.0 has no `Layer.prototype.handle_request`/`handle_error`; the router invokes `layer.handle` directly, so wrapping the prototype traced nothing and dropped middleware, route-handler, and code-origin spans there. Replace `layer.handle` in place (arity preserved so the host still routes error handlers) when the layer exposes no dispatch method. express 4.3.0+, express 5, the router package, and loopback keep `handle` pristine via the prototype wraps. Also point the router-helper spec at the renamed `setLayerMeta` helper.
express-async-errors redefines `handle` as a getter/setter that stores the wrapped handler in `__handle` and patches `handle` only, never `handle_request`. On express 4.3.0+ the tracer's prototype-dispatch wrap therefore has to survive that patch, the arity gate has to read the real 3-arg handler, and a rejected async handler has to reach the wrapped `next` as `next(error)`. Pin that path so a regression surfaces here instead of in a downstream tracer test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c90dd1dd6c
ℹ️ 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".
…ress <4.6.0 On express <4.6.0 the router has no `Layer` prototype dispatch: it runs `layer.handle` directly and, on a synchronous throw, catches outside the layer and calls its own `next(error)` — never the wrapped `next` the tracer installed. The prototype-dispatch rewrite dropped the legacy handle wrap's catch, so a throwing middleware or route handler no longer published `middleware:error` / `middleware:next` / `middleware:finish`: the span lost its error tag and lingered on the stack until request finish. Restore the catch in `wrapLegacyHandle` to publish error/next/finish and rethrow, mirroring `wrapNext`. The prototype hosts (express >=4.6.0, express 5, router) already route the throw through the wrapped next, so only the legacy path needs it. Also correct the version boundary in the surrounding comments: `handle_request` landed in express 4.6.0, not 4.3.0, so the legacy path covers `>=4.0.0 <4.6.0`. Refs: #9067 (comment)
szegedi
left a comment
There was a problem hiding this comment.
Few notes surfaced by automated review:
- async rejections now fire
exitbeforeerror/next/finish, which is a bit of a behavior change, but I think the net effect is better as async middleware errors are now tagged on the middleware span. - a handler that calls
next()and then rejects double-callswrappedNexte.g.async (req, res, next) => { next(); await bg() }wherebg()rejects. The explicitnext()finishes and pops the middleware span; the host's rejection.thenthen callswrappedNext(error)a second time, which tags error on the parent (context.span) since the middleware stack is now empty. The finish re-fire is a guarded no-op (no crash). Old code routed the rejection through the host's raw next, so this second tracer pass didn't happen. Pathological input, low impact (request-level error tagging is unchanged), but a real behavioral delta.
None of these I think need to be addressed, but it's not a bad idea to at least have a record of them in form of this review comment on the PR.
| return { wrapLayerRequest, wrapLayerError, wrapLegacyHandle } | ||
| } | ||
|
|
||
| function hasLayerDispatch (layer) { |
There was a problem hiding this comment.
Agent probably missed the “All new methods should receive a full JSDoc comment.” instruction from AGENTS.md.
There was a problem hiding this comment.
Yes... I want to add a lint rule at some point. I believe that is the only way to get this right
A handler that calls `next()` and then rejects (`async (req, res, next) => {
next(); await bg() }` where `bg()` rejects) makes the host invoke the same
wrapped `next` twice: the clean `next()` finishes and pops the middleware span,
then the host's rejection pass calls `next(error)` again. Since the middleware
stack is already empty, that second pass tagged the error on the parent span,
inventing a request-level error the middleware never surfaced.
Guard the wrapped continuation so its error/next/finish publishes fire at most
once per dispatch; the second call still forwards to the host chain untouched.
Add the missing JSDoc on `hasLayerDispatch`.
|
@szegedi I added a guard against calling next twice. This is actually undefined behavior for express and similar and the former behavior used the last call, if I am not mistaken, which is actually wrong. Now we use the first one, that is correct. I am going to open a follow-up PR that adds a tag to surface the information that next was called more than once. |
* refactor(express): trace middleware via layer prototype dispatch Middleware tracing replaced `layer.handle` with a wrapper and re-exposed the user handler through an enumerable `_datadog_orig` back-reference, the only property loopback's `_findLayerByHandler` could follow to tag a layer with its phase. That ties the tracer to loopback's private reflection heuristic, where the property name and its enumerability are both load-bearing. Wrap the host `Layer` prototype dispatch (`handle_request`/`handle_error`, and `handleRequest`/`handleError` on router >=2) and read per-layer metadata from a WeakMap, leaving `layer.handle` the user's function. loopback's first lookup (`layer.handle === handler`) then matches with no tracer-specific contract, so the back-reference, the `express-async-errors` `__handle` branch, and the `_name` write-back onto the user handler are no longer needed. A synchronous throw or a rejected promise becomes `next(error)` in the host dispatch, so the wrapped `next` captures both without a tracer-side try/catch. Refs: #9062 * fix(express): trace middleware on express <4.3.0 (no prototype dispatch) express <4.3.0 has no `Layer.prototype.handle_request`/`handle_error`; the router invokes `layer.handle` directly, so wrapping the prototype traced nothing and dropped middleware, route-handler, and code-origin spans there. Replace `layer.handle` in place (arity preserved so the host still routes error handlers) when the layer exposes no dispatch method. express 4.3.0+, express 5, the router package, and loopback keep `handle` pristine via the prototype wraps. Also point the router-helper spec at the renamed `setLayerMeta` helper. * test(router): cover express-async-errors on prototype-dispatch hosts express-async-errors redefines `handle` as a getter/setter that stores the wrapped handler in `__handle` and patches `handle` only, never `handle_request`. On express 4.3.0+ the tracer's prototype-dispatch wrap therefore has to survive that patch, the arity gate has to read the real 3-arg handler, and a rejected async handler has to reach the wrapped `next` as `next(error)`. Pin that path so a regression surfaces here instead of in a downstream tracer test. * fix(express): restore error publishing for throwing middleware on express <4.6.0 On express <4.6.0 the router has no `Layer` prototype dispatch: it runs `layer.handle` directly and, on a synchronous throw, catches outside the layer and calls its own `next(error)` — never the wrapped `next` the tracer installed. The prototype-dispatch rewrite dropped the legacy handle wrap's catch, so a throwing middleware or route handler no longer published `middleware:error` / `middleware:next` / `middleware:finish`: the span lost its error tag and lingered on the stack until request finish. Restore the catch in `wrapLegacyHandle` to publish error/next/finish and rethrow, mirroring `wrapNext`. The prototype hosts (express >=4.6.0, express 5, router) already route the throw through the wrapped next, so only the legacy path needs it. Also correct the version boundary in the surrounding comments: `handle_request` landed in express 4.6.0, not 4.3.0, so the legacy path covers `>=4.0.0 <4.6.0`. Refs: #9067 (comment) * fix(express): publish middleware next/finish once per continuation A handler that calls `next()` and then rejects (`async (req, res, next) => { next(); await bg() }` where `bg()` rejects) makes the host invoke the same wrapped `next` twice: the clean `next()` finishes and pops the middleware span, then the host's rejection pass calls `next(error)` again. Since the middleware stack is already empty, that second pass tagged the error on the parent span, inventing a request-level error the middleware never surfaced. Guard the wrapped continuation so its error/next/finish publishes fire at most once per dispatch; the second call still forwards to the host chain untouched. Add the missing JSDoc on `hasLayerDispatch`.
* refactor(express): trace middleware via layer prototype dispatch Middleware tracing replaced `layer.handle` with a wrapper and re-exposed the user handler through an enumerable `_datadog_orig` back-reference, the only property loopback's `_findLayerByHandler` could follow to tag a layer with its phase. That ties the tracer to loopback's private reflection heuristic, where the property name and its enumerability are both load-bearing. Wrap the host `Layer` prototype dispatch (`handle_request`/`handle_error`, and `handleRequest`/`handleError` on router >=2) and read per-layer metadata from a WeakMap, leaving `layer.handle` the user's function. loopback's first lookup (`layer.handle === handler`) then matches with no tracer-specific contract, so the back-reference, the `express-async-errors` `__handle` branch, and the `_name` write-back onto the user handler are no longer needed. A synchronous throw or a rejected promise becomes `next(error)` in the host dispatch, so the wrapped `next` captures both without a tracer-side try/catch. Refs: #9062 * fix(express): trace middleware on express <4.3.0 (no prototype dispatch) express <4.3.0 has no `Layer.prototype.handle_request`/`handle_error`; the router invokes `layer.handle` directly, so wrapping the prototype traced nothing and dropped middleware, route-handler, and code-origin spans there. Replace `layer.handle` in place (arity preserved so the host still routes error handlers) when the layer exposes no dispatch method. express 4.3.0+, express 5, the router package, and loopback keep `handle` pristine via the prototype wraps. Also point the router-helper spec at the renamed `setLayerMeta` helper. * test(router): cover express-async-errors on prototype-dispatch hosts express-async-errors redefines `handle` as a getter/setter that stores the wrapped handler in `__handle` and patches `handle` only, never `handle_request`. On express 4.3.0+ the tracer's prototype-dispatch wrap therefore has to survive that patch, the arity gate has to read the real 3-arg handler, and a rejected async handler has to reach the wrapped `next` as `next(error)`. Pin that path so a regression surfaces here instead of in a downstream tracer test. * fix(express): restore error publishing for throwing middleware on express <4.6.0 On express <4.6.0 the router has no `Layer` prototype dispatch: it runs `layer.handle` directly and, on a synchronous throw, catches outside the layer and calls its own `next(error)` — never the wrapped `next` the tracer installed. The prototype-dispatch rewrite dropped the legacy handle wrap's catch, so a throwing middleware or route handler no longer published `middleware:error` / `middleware:next` / `middleware:finish`: the span lost its error tag and lingered on the stack until request finish. Restore the catch in `wrapLegacyHandle` to publish error/next/finish and rethrow, mirroring `wrapNext`. The prototype hosts (express >=4.6.0, express 5, router) already route the throw through the wrapped next, so only the legacy path needs it. Also correct the version boundary in the surrounding comments: `handle_request` landed in express 4.6.0, not 4.3.0, so the legacy path covers `>=4.0.0 <4.6.0`. Refs: #9067 (comment) * fix(express): publish middleware next/finish once per continuation A handler that calls `next()` and then rejects (`async (req, res, next) => { next(); await bg() }` where `bg()` rejects) makes the host invoke the same wrapped `next` twice: the clean `next()` finishes and pops the middleware span, then the host's rejection pass calls `next(error)` again. Since the middleware stack is already empty, that second pass tagged the error on the parent span, inventing a request-level error the middleware never surfaced. Guard the wrapped continuation so its error/next/finish publishes fire at most once per dispatch; the second call still forwards to the host chain untouched. Add the missing JSDoc on `hasLayerDispatch`.
Summary
loopback's phase-based middleware sorting relies on
_findLayerByHandlermapping each layer back to the user handler. The router instrumentation replacedlayer.handlewith a wrapper, then (in #9062) re-exposed the handler through an enumerable_datadog_origback-reference so that lookup could still find it — keeping the tracer coupled to loopback's private reflection heuristic, where the property name and its enumerability are both load-bearing.This replaces that shim: trace middleware by wrapping the host
Layerprototype dispatch and reading per-layer metadata from aWeakMap, leavinglayer.handlethe user's function.router.js—wrapStackannotates each layer in aWeakMap(annotateLayer) instead of replacing its handle; a newcreateLayerDispatchWrappers(name)wrapshandle_request/handle_error(andhandleRequest/handleErroron router>=2). DropswrapLayerHandle, the arity-split wrappers, the_datadog_origback-reference, the__handlebranch, and the_namewrite-back onto the user handler.express.js— wraps express 4's bundledLayerprototype; express 5 / standalone router flow through therouter-package layer hook.Why
loopback already works on master via #9062's
_datadog_origproperty, so this is a no-behavior-change refactor. Keepinglayer.handlepristine makes loopback's first lookup (layer.handle === handler) match with no tracer-specific contract, removing the coupling to itsfor-inreflection heuristic entirely. As a side effect the design is lighter: one shared monomorphic dispatch wrapper replaces a per-layer closure, and the hot path drops itstry/catch(a synchronous throw or rejected promise becomesnext(error)in the host dispatch, which the wrappednextalready captures).Test plan
packages/datadog-instrumentations/test/router.spec.jsrewritten to the dispatch model — 28 passing (pristine handle, arity gates, no-subscriber fast paths, host-converted errors, re-entrancy guard).WeakMapis within noise of (slightly faster than) the per-layer closure — no regression.routermatrix (>=1 <2,>=2), and the loopback 2.x/3.x suite — version matrices are not installable locally; the loopback re-sorting canary (spans[4]resourcehandleDD) is the gate.express-async-errorscompatibility —handleis its getter/setter; the dispatch readsthis.handleand the.lengtharity gate holds, so the__handlespecial case is gone.router-packageLayeraddHook bodies and the nested-router mount line are covered by the express/router plugin integration jobs, not the unit spec.Refs: #9062