perf: append the undici traceparent without addHeader()

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);
   }

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

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

test/test-undici.js

@@ -4,13 +4,17 @@ const assert = require('node:assert');
 const { after, before, describe, it } = require('node:test');
 const {
   startTracing,
+  startOTelCollector,
   startHTTPServer,
   flushAndCollect,
   getClientSpan,
   getServerSpan,
   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);