diff --git a/lib/instrumentations.js b/lib/instrumentations.js index 5cb80ff..ea63bf1 100644 --- a/lib/instrumentations.js +++ b/lib/instrumentations.js @@ -344,22 +344,33 @@ function onHttp2ServerStreamError({ stream, error }) { } } -// Undici's addHeader appends rather than replaces; update an existing -// header in place so a caller-supplied value is overridden, not -// duplicated. +// undici keeps a request's headers as a flat array of alternating names +// and values, and its addHeader() appends rather than replaces. An existing +// header is updated in place, so a caller's value is overridden instead of +// duplicated. Only traceparent skips addHeader() when it is appended: +// microtel makes its value, so addHeader()'s validation has nothing to +// catch, and every request carries one. tracestate and baggage carry +// values from elsewhere, so they keep that validation. Headers in any other +// shape go through addHeader(). function setUndiciHeader(request, name, value) { const headers = request.headers; + if (!Array.isArray(headers) || (headers.length & 1) !== 0) { + request.addHeader(name, value); + return; + } for (let i = 0; i < headers.length; i += 2) { if (headers[i] === name) { headers[i + 1] = value; return; } } - request.addHeader(name, value); + if (name === 'traceparent') headers.push(name, value); + else request.addHeader(name, value); } function removeUndiciHeader(request, name) { const headers = request.headers; + if (!Array.isArray(headers)) return; for (let i = headers.length - 2; i >= 0; i -= 2) { if (headers[i] === name) headers.splice(i, 2); } diff --git a/test/fixtures/undici-headers-fallback-child.js b/test/fixtures/undici-headers-fallback-child.js new file mode 100644 index 0000000..5fc61e2 --- /dev/null +++ b/test/fixtures/undici-headers-fallback-child.js @@ -0,0 +1,44 @@ +'use strict'; + +// Spawned with JSON init options as argv[2] and a shape as argv[3]: makes +// the first read of request.headers return something other than a flat +// array of name and value pairs, so the undici instrumentation falls back +// to addHeader(), while every later read sees the real array, where the +// fallback lands the header. Subscribed before init() so this handler runs +// ahead of the instrumentation's. + +const dc = require('node:diagnostics_channel'); +const http = require('node:http'); +const { init, flush } = require(process.env.MICROTEL_PATH); + +// argv[3] picks what the first read returns: an object, or an array with an +// odd number of entries. +const first = process.argv[3] === 'odd' ? ['sentinel'] : {}; + +dc.subscribe('undici:request:create', ({ request }) => { + const headers = request.headers; + let reads = 0; + Object.defineProperty(request, 'headers', { + configurable: true, + get: () => (reads++ === 0 ? first : headers), + }); +}); + +init(JSON.parse(process.argv[2])); + +let traceparent; +const target = http.createServer((req, res) => { + traceparent = req.headers.traceparent; + res.writeHead(200); + res.end('ok'); +}); + +target.listen(0, '127.0.0.1', async () => { + const res = await fetch(`http://127.0.0.1:${target.address().port}/down`); + await res.text(); + flush(); + target.close(() => { + console.log(traceparent === undefined ? 'none' : traceparent); + console.log('done'); + }); +}); diff --git a/test/fixtures/undici-headers-spy-child.js b/test/fixtures/undici-headers-spy-child.js new file mode 100644 index 0000000..3caecb4 --- /dev/null +++ b/test/fixtures/undici-headers-spy-child.js @@ -0,0 +1,63 @@ +'use strict'; + +// Spawned with JSON init options as argv[2]: makes a fetch whose span +// carries a tracestate and baggage, and prints the header names microtel +// passed to addHeader() and the trace headers the fetch target received. + +const dc = require('node:diagnostics_channel'); +const http = require('node:http'); +const { init, flush } = require(process.env.MICROTEL_PATH); + +const added = []; +// Subscribed before init(), so this runs ahead of microtel's handler. +dc.subscribe('undici:request:create', ({ request }) => { + const addHeader = request.addHeader; + request.addHeader = function (name, value) { + added.push(name); + return addHeader.call(this, name, value); + }; +}); + +init(JSON.parse(process.argv[2])); + +let received; +const target = http.createServer((req, res) => { + const { traceparent, tracestate, baggage } = req.headers; + received = { traceparent, tracestate, baggage }; + res.end('ok'); +}); +const frontend = http.createServer(async (req, res) => { + const fetched = await fetch(`http://127.0.0.1:${target.address().port}/`); + await fetched.text(); + res.end('ok'); +}); + +const listen = (server) => + new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + +(async () => { + await listen(target); + await listen(frontend); + await new Promise((resolve) => + http.get( + `http://127.0.0.1:${frontend.address().port}/`, + { + headers: { + traceparent: + '00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01', + tracestate: 'rojo=00f067aa0ba902b7', + baggage: 'user=alice', + }, + }, + (res) => { + res.resume(); + res.on('end', resolve); + }, + ), + ); + flush(); + frontend.close(); + target.close(); + http.globalAgent.destroy(); + console.log(JSON.stringify({ added, received })); +})(); diff --git a/test/test-undici.js b/test/test-undici.js index 94dafb5..6511808 100644 --- a/test/test-undici.js +++ b/test/test-undici.js @@ -4,6 +4,7 @@ const assert = require('node:assert'); const { after, before, describe, it } = require('node:test'); const { startTracing, + startOTelCollector, startHTTPServer, flushAndCollect, getClientSpan, @@ -11,6 +12,9 @@ const { rawRequest, spanAttrs, closeAll, + fixture, + getSpans, + spawnMicrotel, } = require('./common/otel'); describe('otel undici spans', () => { @@ -217,6 +221,128 @@ describe('otel undici spans', () => { ); }); + it('forwards the full trace context on a fetch', async () => { + const incomingTraceId = 'c'.repeat(32); + const incomingTracestate = 'vendor=state,other=1'; + const incomingBaggage = 'key=value,other=2'; + + 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, + baggage: req.headers.baggage, + }; + res.writeHead(200); + res.end('ok'); + }); + + // A fetch with no trace headers of its own, so every injected header + // takes the appended path rather than overriding a caller-supplied + // value. + const frontend = await startHTTPServer((req, res) => { + fetch(`http://127.0.0.1:${fetchTarget.port}/down`).then((fr) => { + fr.text().then(() => { + res.writeHead(200); + res.end('ok'); + }); + }); + }); + + await rawRequest(frontend.port, '/up', { + traceparent: `00-${incomingTraceId}-0123456789abcdef-01`, + tracestate: incomingTracestate, + baggage: incomingBaggage, + }); + + 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 be forwarded verbatim', + ); + assert.strictEqual( + seen.baggage, + incomingBaggage, + 'The inherited baggage must be forwarded verbatim', + ); + }); + + it('validates tracestate and baggage through addHeader', async () => { + // traceparent is the only header microtel appends without addHeader(), + // since only its value is always microtel's own. + const { code, stdout, stderr } = await spawnMicrotel([ + fixture('undici-headers-spy-child.js'), + // The fixture's own http.get stands in for a caller, so microtel must + // not replace its trace headers. + JSON.stringify({ + endpoint: 'http://127.0.0.1:1', + exclude: 'node:http/client', + }), + ]); + + assert.strictEqual(code, 0, stderr); + const { added, received } = JSON.parse(stdout); + assert.deepStrictEqual(added, ['tracestate', 'baggage']); + assert.partialDeepStrictEqual(received, { + tracestate: 'rojo=00f067aa0ba902b7', + baggage: 'user=alice', + }); + assert.match( + received.traceparent, + /^00-0af7651916cd43dd8448eb211c80319c-[0-9a-f]{16}-01$/, + ); + }); + + it('falls back to addHeader when the headers are not a flat array', async () => { + const collector = await startOTelCollector(); + + // The collector is closed even when an assertion fails, or its open + // server would keep the test process alive. + try { + // A child process makes the first read of request.headers return an + // object or an odd-length array, so the appended headers must go + // through addHeader(). + for (const shape of ['object', 'odd']) { + const { code, stdout, stderr } = await spawnMicrotel([ + fixture('undici-headers-fallback-child.js'), + JSON.stringify({ + endpoint: `http://127.0.0.1:${collector.port}`, + filter: 'node:undici', + }), + shape, + ]); + + // The assertions on the child's exit come first: a child that dies + // before its export never delivers a payload, and awaiting the + // collector up front would hang the suite on it. + assert.strictEqual(code, 0, shape); + assert.strictEqual(stderr, '', shape); + const [traceparent] = stdout.trim().split('\n'); + assert.match( + traceparent, + /^00-[0-9a-f]{32}-[0-9a-f]{16}-0[23]$/, + `addHeader must land a valid traceparent on the wire (${shape})`, + ); + + // The child has exited, so the export its flush started has + // completed. + const spans = getSpans(await collector.next()); + assert.ok(getClientSpan(spans, '/down'), shape); + } + } finally { + await collector.close(); + } + }); + it('parents fetch spans to the server span in the same trace', async () => { const incomingTraceId = 'a'.repeat(32);