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', + ); + }); +});