feat: undici and fetch() instrumentation
#2858
Conversation
github-actions
bot
added
the
agent-nodejs
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
…on errored requests
…text propagation to work using UndiciConnection with a node that doesn't have diagnostics_channels
….561039ms' in GH actions
…ntal-fetch; test undici instrumentation in node <v14.17.0 if the diagnostics_channel polyfill module has been installed
…change; remove dupe URL parsing
|
run module tests for undici,@elastic/elasticsearch |
|
run module tests for undici,@elastic/elasticsearch |
| @@ -191,7 +191,7 @@ tape.test('contextPropagationOnly', function (suite) { | |||
| // stacktrace collection when contextPropagationOnly=true. It isn't a | |||
| // perfect way to test that. | |||
| const durationMs = duration[0] / 1e3 + duration[1] / 1e6 | |||
| const THRESHOLD_MS = 3 // Is this long enough for slow CI? | |||
| const THRESHOLD_MS = 5 // Is this long enough for slow CI? | |||
There was a problem hiding this comment.
REVIEW NOTE: This is an unrelated change. I hit a spurious test failure with this tight threshold, so I bumped the threshold to make it less likely to hit it in CI.
There was a problem hiding this comment.
For example this just failed on "main": https://github.com/elastic/apm-agent-nodejs/runs/7771135311?check_suite_focus=true
not ok 16 captureError is fast (<3ms): 3.015229ms
There was a problem hiding this comment.
I ran this through some rudimentary tests -- spans are made, tracecontext is propagated/distributed tracing functions -- things seem to work and having this in place puts us ahead of most folks w/r/t undici and any bleeding edge folks bold enough to incorporate the new HTTP client into their projects (:wave: @ elastic client team)
Fine with this going in as is but I do have a question re: what looks like double instrumentation below, and I'm not sure if the lack of a CHANGELOG entry is deliberate or just an oversight.
|
|
||
| if (nodeHasInstrumentableFetch && this._isModuleEnabled('undici')) { | ||
| this._log.debug('instrumenting fetch') | ||
| undiciInstr.instrumentUndici(this._agent) |
There was a problem hiding this comment.
(I'm afraid to ask but) -- what's driving the need for this manual call to instrumentUndici. Naively -- it seems like adding undichi to MODULES will always load lib/instrumentation/modules/undici.js which also calls instrumentUndici. What's the weird case that leads us to need this second, earlier, call to instrumentUndici?
There was a problem hiding this comment.
adding undichi to MODULES will always load lib/instrumentation/modules/undici.js
It will load lib/instrumentation/modules/undici.js, yes, but the shimming (shimUndici) isn't called unless the require-in-the-middle hook sees a require('undici').
What's the weird case that leads ...
To instrument node's core fetch() implementation we need to enable the undici instrumentation even if there is no require('undici') anywhere. This is because node's core fetch() uses undici as an esbuild bundle: https://github.com/nodejs/node/blob/main/deps/undici/undici.js
That we can even instrument that is down to the hooks being via diagnostics_channel rather than needing to monkey-patch.
And to avoid double-instrumenting, there is a guard at the start of instrumentUndici:
function instrumentUndici (agent) {
if (isInstrumented) {
return
}
isInstrumented = true
...I'm not sure if the lack of a CHANGELOG entry is deliberate or just an oversight.
Oh, that is totally an oversight. Thanks for pointing that out!
Closes: #2383