From 23e1d14a58bf3beccb83c6da5575cfb26413bdf9 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 26 Oct 2026 13:31:50 -0400 Subject: [PATCH] feat: add undici and fetch instrumentation One CLIENT span per fetch() or request() call, with the same shape as the node:http client spans: http.request.method and url.full (origin + path) attributes, server.address set to the origin, server.port derived from it, and the span ended on trailers, which fire once the response body completes. Trace context injection replaces rather than appends headers, since undici's addHeader appends, and a stale caller-supplied tracestate is removed when the span has none to inherit. The filter values node:undici and node:fetch enable it, as an alias pair. Signed-off-by: Bryan English --- README.md | 20 +++-- lib/instrumentations.js | 92 ++++++++++++++++---- test/test-init.js | 24 +++++ test/test-undici.js | 188 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 299 insertions(+), 25 deletions(-) create mode 100644 test/test-undici.js diff --git a/README.md b/README.md index d8605b1..e852ffd 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ started. Invalid options throw. | `serviceName` | string | `unknown_service:node` | Service name in exported resource attributes. The default follows the OpenTelemetry [semantic conventions](https://opentelemetry.io/docs/specs/semconv/general/trace/) for low-cardinality service names. | | `sdkName` | string | `microtel` | `telemetry.sdk.name` resource attribute, and the name of the instrumentation scope on exported spans. | | `sdkVersion` | string | this package's version | `telemetry.sdk.version` resource attribute, and the scope version. | -| `filter` | string | all modules | Comma-separated list of modules to instrument. Supported values: `node:http`, `node:http/client`, `node:http/server`. | +| `filter` | string | all modules | Comma-separated list of modules to instrument. Supported values: `node:http`, `node:http/client`, `node:http/server`, and `node:undici`. `node:fetch` is an alias for `node:undici`: both cover all undici requests, including `fetch()`, and either one enables them. | | `exclude` | string | none | Comma-separated list of modules to exclude from instrumentation. Same values as `filter`; takes precedence over it. | | `maxBufferSize` | number | `100` | Number of spans buffered before an immediate flush is triggered. Must be a positive integer. | | `flushInterval` | number | `10000` | Milliseconds between periodic flushes. Must be a positive integer. | @@ -121,11 +121,14 @@ port that is not a decimal number is omitted. ### Undici/Fetch client -Not implemented yet. The instrumentation subscribes to the -`undici:request:*` diagnostics channels and produces one CLIENT span per -request, with the same attributes as the HTTP client spans plus -`server.port`; the plan is documented as a TODO in -`lib/instrumentations.js`. +A span with kind `CLIENT` is created for each outgoing request made via +`fetch()` or undici's `request()`. Error status, `exception` event behavior, +and span end timing are the same as for the HTTP client spans above. Requests +that fail before they are dispatched, such as a refused connection, produce +no span: the undici channels only fire once the request is underway. The +injected `traceparent` replaces any caller-supplied value rather than +appending to it, and a stale `tracestate` is removed when the span has none +to inherit. Attributes set on undici/fetch client spans: @@ -152,9 +155,8 @@ fresh trace is started instead. The `tracestate` header, when present, is attached to the span and forwarded verbatim to outgoing requests. It is never parsed or modified. * **Outgoing requests**: The `traceparent` header is injected into outgoing -HTTP requests, together with the stored `tracestate` when one -is present, enabling distributed tracing across services. Undici and -fetch requests are covered once that instrumentation lands. +HTTP and undici/fetch requests, together with the stored `tracestate` when +one is present, enabling distributed tracing across services. ## Benchmarks diff --git a/lib/instrumentations.js b/lib/instrumentations.js index 398b721..96177a6 100644 --- a/lib/instrumentations.js +++ b/lib/instrumentations.js @@ -206,22 +206,73 @@ function onClientRequestError({ request, error }) { failClientSpan(span, error); } -// TODO(bengl): undici/fetch instrumentation. Subscribing to the -// undici:request:create, undici:request:headers, undici:request:trailers, -// and undici:request:error channels yields one CLIENT span per fetch() -// or request() call, with the same shape as the node:http client spans -// above: http.request.method and url.full (origin + path) attributes, -// server.address set to the origin, server.port derived from it, and the -// span ended on trailers, which fire once the response body completes. -// The headers channel applies the response status; the error channel -// reuses failClientSpan. Trace context injection must replace rather than -// append headers: undici's addHeader appends, so a caller-supplied -// traceparent has to be updated in place or it arrives duplicated. The -// filter values node:undici and node:fetch enable it, as an alias pair. -// A working implementation, including its test, exists in -// nodejs/node#66587 (lib/internal/otel/instrumentations.js and -// test/parallel/test-otel-undici.js); the PR's diff stays viewable even -// once it is closed. +// Undici's addHeader appends rather than replaces; update an existing +// header in place so a caller-supplied value is overridden, not +// duplicated. +function setUndiciHeader(request, name, value) { + const headers = request.headers; + for (let i = 0; i < headers.length; i += 2) { + if (headers[i] === name) { + headers[i + 1] = value; + return; + } + } + request.addHeader(name, value); +} + +function removeUndiciHeader(request, name) { + const headers = request.headers; + for (let i = headers.length - 2; i >= 0; i -= 2) { + if (headers[i] === name) headers.splice(i, 2); + } +} + +function onUndiciRequestCreate({ request }) { + const parent = spanStorage.getStore(); + + const method = request.method || 'GET'; + const span = new Span(method, SPAN_KIND_CLIENT, { parent }); + + span.setAttribute('http.request.method', method); + + const { origin } = request; + const path = request.path; + if (origin) { + span.setAttribute('server.address', origin); + // The origin omits the port when it is the scheme default. + const parsed = new URL(origin); + span.setAttribute( + 'server.port', + Number(parsed.port) || (parsed.protocol === 'https:' ? 443 : 80), + ); + } + span.setAttribute('url.full', `${origin}${path}`); + + request[kSpan] = span; + + if (request.addHeader) { + setUndiciHeader(request, 'traceparent', span.toTraceparent()); + if (span.tracestate) + setUndiciHeader(request, 'tracestate', span.tracestate); + else removeUndiciHeader(request, 'tracestate'); + } +} + +function onUndiciRequestHeaders({ request, response }) { + const span = request[kSpan]; + if (span == null) return; + + applyClientResponse(span, response.statusCode); +} + +// undici:request:trailers fires when the response completes, including its +// body, so the span duration covers the full transfer. +function onUndiciRequestTrailers({ request }) { + const span = request[kSpan]; + if (span == null) return; + + span.end(); +} function enableInstrumentations() { spanStorage = getSpanStorage(); @@ -236,6 +287,15 @@ function enableInstrumentations() { dc.subscribe('http.client.response.finish', onHttpClientResponseFinish); dc.subscribe('http.client.request.error', onClientRequestError); } + + // node:fetch is an alias for node:undici: both enable the same + // channels, which cover every undici request, fetch() included. + if (isModuleEnabled('node:undici') || isModuleEnabled('node:fetch')) { + dc.subscribe('undici:request:create', onUndiciRequestCreate); + dc.subscribe('undici:request:headers', onUndiciRequestHeaders); + dc.subscribe('undici:request:trailers', onUndiciRequestTrailers); + dc.subscribe('undici:request:error', onClientRequestError); + } } module.exports = { diff --git a/test/test-init.js b/test/test-init.js index 83dc160..eccc7f6 100644 --- a/test/test-init.js +++ b/test/test-init.js @@ -187,6 +187,30 @@ describe('otel init activation', () => { ); }); + it('filter accepts node:fetch as an alias for undici', async () => { + const { code, stdout } = await spawnMicrotel([ + fixture('state-child.js'), + JSON.stringify([ + { + endpoint: 'http://127.0.0.1:1', + filter: 'node:fetch', + }, + ]), + 'node:fetch,node:http', + ]); + + assert.strictEqual(code, 0); + const lines = stdout.trim().split('\n'); + assert.match( + lines.find((l) => l.startsWith('node:fetch:')), + /node:fetch:true/, + ); + assert.match( + lines.find((l) => l.startsWith('node:http:')), + /node:http:false/, + ); + }); + it('exclude disables modules without touching the rest', async () => { const { code, stdout } = await spawnMicrotel([ fixture('state-child.js'), diff --git a/test/test-undici.js b/test/test-undici.js new file mode 100644 index 0000000..2655464 --- /dev/null +++ b/test/test-undici.js @@ -0,0 +1,188 @@ +'use strict'; + +const assert = require('node:assert'); +const { after, before, describe, it } = require('node:test'); +const { + startTracing, + startHTTPServer, + flushAndCollect, + getClientSpan, + rawRequest, + spanAttrs, + closeAll, +} = require('./common/otel'); + +describe('otel undici spans', () => { + let collector; + + before(async () => { + collector = await startTracing({ + filter: 'node:http,node:undici', + }); + }); + + after(() => collector.close()); + + it( + 'creates client spans for fetch requests and overrides existing ' + + 'trace context headers', + async () => { + const receivedTraceparent = {}; + const target = await startHTTPServer((req, res) => { + receivedTraceparent[req.url] = req.headers.traceparent; + res.writeHead(200); + res.end('ok'); + }); + + try { + const res = await fetch(`http://127.0.0.1:${target.port}/fetch-test`); + await res.text(); + } catch {} + + // A caller-supplied traceparent must be overridden, not duplicated: + // undici's addHeader appends, so a duplicate would arrive as a single + // comma-separated value. + try { + const res = await fetch( + `http://127.0.0.1:${target.port}/override-test`, + { + headers: { + traceparent: + '00-00000000000000000000000000000000-' + '0000000000000000-00', + }, + }, + ); + await res.text(); + } catch {} + + const spans = await flushAndCollect(collector); + await target.close(); + + assert.ok(spans.length > 0, 'Expected at least one span from fetch'); + const clientSpan = getClientSpan(spans, '/fetch-test'); + assert.ok(clientSpan, 'Expected a CLIENT span from fetch'); + + assert.partialDeepStrictEqual(spanAttrs(clientSpan), { + 'http.request.method': 'GET', + 'server.address': `http://127.0.0.1:${target.port}`, + 'server.port': String(target.port), + 'url.full': `http://127.0.0.1:${target.port}/fetch-test`, + 'http.response.status_code': '200', + }); + + const overridden = receivedTraceparent['/override-test']; + assert.ok(overridden, 'Expected a traceparent on the override request'); + assert.match(overridden, /^00-[0-9a-f]{32}-[0-9a-f]{16}-0[01]$/); + assert.ok( + !overridden.includes(','), + 'traceparent must be overridden, not duplicated', + ); + }, + ); + + it('creates an error span for a failed fetch', async () => { + // A response that dies mid-transfer must end the span with the error + // status and an exception event, through the undici:request:error + // channel. A fetch that fails to connect never dispatches, so it + // produces no span at all. + const target = await startHTTPServer((req, res) => { + res.writeHead(200); + res.write('partial'); + setTimeout(() => res.destroy(), 50); + }); + + await fetch(`http://127.0.0.1:${target.port}/destroy`) + .then((res) => res.text()) + .catch(() => {}); + + const spans = await flushAndCollect(collector); + await target.close(); + + const errorSpan = getClientSpan(spans, '/destroy'); + assert.ok(errorSpan, 'Expected an error client span for the failed fetch'); + + assert.ok(errorSpan.status, 'Error span should have status'); + assert.strictEqual(errorSpan.status.code, 2); // STATUS_ERROR + assert.ok( + errorSpan.events?.some((e) => e.name === 'exception'), + 'Should have an exception event', + ); + }); + + it('overrides a stale tracestate with the inherited one', async () => { + const incomingTraceId = 'a'.repeat(32); + const incomingTracestate = 'vendor=inherited'; + + let seen; + // The fetch target records the trace context headers it receives. + const fetchTarget = await startHTTPServer((req, res) => { + seen = { + traceparent: req.headers.traceparent, + tracestate: req.headers.tracestate, + }; + res.writeHead(200); + res.end('ok'); + }); + + // A fetch made while handling a traced request inherits the incoming + // tracestate and must override the caller-supplied stale one. + const frontend = await startHTTPServer((req, res) => { + fetch(`http://127.0.0.1:${fetchTarget.port}/down`, { + headers: { tracestate: 'stale=caller-value' }, + }).then((fr) => { + fr.text().then(() => { + res.writeHead(200); + res.end('ok'); + }); + }); + }); + + await rawRequest(frontend.port, '/up', { + traceparent: `00-${incomingTraceId}-0123456789abcdef-01`, + tracestate: incomingTracestate, + }); + + await flushAndCollect(collector); + await closeAll(fetchTarget, frontend); + + assert.ok(seen?.traceparent, 'Expected a traceparent on the fetch'); + assert.ok( + seen.traceparent.includes(incomingTraceId), + 'The fetch must carry the incoming trace ID', + ); + assert.strictEqual( + seen.tracestate, + incomingTracestate, + 'The inherited tracestate must win over the stale one', + ); + }); + + it('removes a stale tracestate when the span has none', async () => { + let receivedTracestate = 'sentinel'; + const target = await startHTTPServer((req, res) => { + receivedTracestate = req.headers.tracestate; + res.writeHead(200); + res.end('ok'); + }); + + try { + const res = await fetch(`http://127.0.0.1:${target.port}/stale`, { + headers: { tracestate: 'vendor=state-for-an-old-trace' }, + }); + await res.text(); + } catch {} + + const spans = await flushAndCollect(collector); + await target.close(); + + assert.ok(getClientSpan(spans, '/stale'), 'Expected a client span'); + + // A stale tracestate from an unrelated trace must not travel with + // the fresh trace context, even though the span has none to inject. + assert.strictEqual( + receivedTracestate, + undefined, + 'stale tracestate must not be forwarded', + ); + }); +});