diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 9bf604db87b..04f4562518f 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -50,6 +50,9 @@ See the <> guide. * Fix undici instrumentation to cope with a bug in undici@6.11.0 where `request.addHeader()` was accidentally removed. (It was re-added in undici@6.11.1.) +* Update undici instrumentation to avoid possibly adding a *second* + 'traceparent' header to outgoing HTTP requests, because this can break + Elasticsearch requests. ({issues}3964[#3964]) [float] ===== Chores diff --git a/lib/instrumentation/modules/undici.js b/lib/instrumentation/modules/undici.js index 2f84e9a320c..4996f4c2379 100644 --- a/lib/instrumentation/modules/undici.js +++ b/lib/instrumentation/modules/undici.js @@ -44,6 +44,9 @@ try { const semver = require('semver'); +// Search an undici@5 request.headers string for a 'traceparent' header. +const headersStrHasTraceparentRe = /^traceparent:/im; + let isInstrumented = false; let spanFromReq = null; let chans = null; @@ -130,17 +133,37 @@ function instrumentUndici(agent) { const propSpan = span || parentRunContext.currSpan() || parentRunContext.currTransaction(); if (propSpan) { - propSpan.propagateTraceContextHeaders( - request, - function (req, name, value) { - if (typeof request.addHeader === 'function') { - req.addHeader(name, value); - } else if (Array.isArray(request.headers)) { - // undici@6.11.0 accidentally, briefly removed `request.addHeader()`. - req.headers.push(name, value); + // Guard against adding a duplicate 'traceparent' header, because that + // breaks ES. https://github.com/elastic/apm-agent-nodejs/issues/3964 + // Dev Note: This cheats a little and assumes the header names to add + // will include 'traceparent'. + let alreadyHasTp = false; + if (Array.isArray(request.headers)) { + // undici@6 + for (let i = 0; i < request.headers.length; i += 2) { + if (request.headers[i].toLowerCase() === 'traceparent') { + alreadyHasTp = true; + break; } - }, - ); + } + } else if (typeof request.headers === 'string') { + // undici@5 + alreadyHasTp = headersStrHasTraceparentRe.test(request.headers); + } + + if (!alreadyHasTp) { + propSpan.propagateTraceContextHeaders( + request, + function (req, name, value) { + if (typeof request.addHeader === 'function') { + req.addHeader(name, value); + } else if (Array.isArray(request.headers)) { + // undici@6.11.0 accidentally, briefly removed `request.addHeader()`. + req.headers.push(name, value); + } + }, + ); + } } if (span) { diff --git a/test/instrumentation/modules/undici/undici.test.js b/test/instrumentation/modules/undici/undici.test.js index 941fffb586f..e4cf13d6a31 100644 --- a/test/instrumentation/modules/undici/undici.test.js +++ b/test/instrumentation/modules/undici/undici.test.js @@ -274,6 +274,45 @@ if (global.AbortController) { }); } +test('undici no duplicate headers', async (t) => { + apm._apmClient.clear(); + const aTrans = apm.startTransaction('aTransName'); + + const url = origin + '/ping'; + const { statusCode, body } = await undici.request(url, { + headers: { + traceparent: 'this-value-is-not-from-elastic-apm-node', + }, + }); + t.equal(statusCode, 200, 'statusCode'); + await consumeResponseBody(body); + + aTrans.end(); + t.error(await promisyApmFlush(), 'no apm.flush() error'); + + t.equal(apm._apmClient.spans.length, 1); + const span = apm._apmClient.spans[0]; + assertUndiciSpan(t, span, url); + + // If there is already a 'traceparent' header in the way, the APM agent + // should at least *not* result in duplicate headers, even if that means + // breaking distributed tracing. + let numTraceparentHeaders = 0; + let numTracestateHeaders = 0; + for (let i = 0; i < lastServerReq.rawHeaders.length; i += 2) { + const k = lastServerReq.rawHeaders[i].toLowerCase(); + if (k === 'traceparent') { + numTraceparentHeaders++; + } else if (k === 'tracestate') { + numTracestateHeaders++; + } + } + t.equal(numTraceparentHeaders, 1, 'just one traceparent header'); + t.equal(numTracestateHeaders, 0, 'tracestate header was skipped'); + + t.end(); +}); + test('teardown', (t) => { undici.getGlobalDispatcher().close(); // Close kept-alive sockets. server.close();