fix(tracer): preserve wrapped function metadata - #10133
Conversation
Overall package sizeSelf size: 8.74 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.4.0 | 127.33 kB | 447.04 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-09-03 14:24:21 Comparing candidate commit 3dd654e in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2313 metrics, 15 unstable metrics.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10133 +/- ##
==========================================
- Coverage 98.63% 98.56% -0.07%
==========================================
Files 1002 1003 +1
Lines 154897 154902 +5
Branches 13523 13244 -279
==========================================
- Hits 152780 152677 -103
- Misses 2117 2225 +108 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:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2447b6984c
ℹ️ 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".
Express classifies middleware by function arity, so tracer.wrap() turns four-argument error handlers into length-0 functions that are skipped. Preserve the wrapped function shape through the existing shimmer helper so callback, return, throw, and constructor behavior remain transparent.
Frozen functions have the same non-writable prototype descriptor as native classes, so assertNotClass rejects valid tracer.wrap() inputs. Let frozen targets through and retain descriptor-copy failures only for immutable wrapper properties.
2447b69 to
3dd654e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dd654eb9a
ℹ️ 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".
| const shimmer = require('../../datadog-shimmer') | ||
|
|
||
| return function (...args) { | ||
| return shimmer.wrapFunction(fn, original => function (...args) { |
There was a problem hiding this comment.
When the newly supported four-argument Express error middleware handles the response without calling next—the normal terminal-error-handler pattern—the last argument is still a function, so this wrapper treats next as a completion callback and never calls done. The unfinished middleware span prevents the trace from flushing under the normal started.length === finished.length path in packages/dd-trace/src/span_processor.js:53; the added integration test masks this by explicitly calling next() before sending its response. Account for middleware completion without requiring next so the motivating Express use case does not lose traces.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a pre existing issue as it seems. That should be handled separately
| const shimmer = require('../../datadog-shimmer') | ||
|
|
||
| return function (...args) { | ||
| return shimmer.wrapFunction(fn, original => function (...args) { |
There was a problem hiding this comment.
When fn defines util.promisify.custom, wrapFunction copies that symbol's descriptor unchanged, so util.promisify(wrapped) returns the original custom promisifier and never executes this wrapper or tracer.trace; before this change, promisifying the metadata-free wrapper still invoked the traced function. The explicit wrapping of the same hook in packages/datadog-instrumentations/src/child_process.js:226-238 confirms that copying it verbatim bypasses instrumentation. Wrap the custom promisifier as well, or avoid exposing the unwrapped hook on the returned function.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This seems to be a pre-existing issue that is something that should be fixed independently
1 participant
What does this PR do?
Preserves the wrapped function’s arity and other function metadata in
tracer.wrap()by using the establisheddatadog-shimmer.wrapFunctionmechanism.This allows Express to recognize wrapped error middleware, which requires a function length of 4.
Motivation
tracer.wrap()currently returns a rest-parameter function with length 0. Express therefore skips wrapped error handlers and returns its fallback 500 response.Additional Notes
The implementation preserves length, name, prototype, own property descriptors, symbols,
thisbinding, callback behavior, synchronous and promise returns, thrown errors, and legacy constructor behavior.Validation:
npm run lint: passingThe full Express suite still has three existing deterministic failures on Express 4.0.0–4.3.0 in its repeated-
next()diagnostic test; these are unrelated to this change.