diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml new file mode 100644 index 00000000000..4b2487c597e --- /dev/null +++ b/.github/workflows/stale.yml @@ -0,0 +1,26 @@ +name: 'Stale PR Bot' +on: + schedule: + - cron: '0 4 * * *' + +permissions: + pull-requests: write + +jobs: + stale: + runs-on: ubuntu-latest + steps: + - uses: actions/stale@5bef64f19d7facfb25b37b414482c7164d639639 # v9.1.0 + with: + days-before-issue-stale: -1 # disabled for issues + + days-before-pr-stale: 90 # 3 months + days-before-pr-close: 14 # 2 weeks + stale-pr-label: "stale" + exempt-pr-labels: "keep-open" + exempt-draft-pr: true + stale-pr-message: | + This pull request has been marked as stale due to 90 days of inactivity. + If this is still relevant, please update or comment to keep it open. + If this should be kept open indefinitely, please apply the label `keep-open`. + Otherwise, it will be automatically closed after 14 days. diff --git a/.github/workflows/system-tests.yml b/.github/workflows/system-tests.yml index 949d74e5b0f..fbd0152fb5a 100644 --- a/.github/workflows/system-tests.yml +++ b/.github/workflows/system-tests.yml @@ -31,7 +31,7 @@ jobs: uses: DataDog/system-tests/.github/workflows/compute-workflow-parameters.yml@main with: library: nodejs - scenarios_groups: essentials,appsec_rasp + scenarios_groups: essentials,appsec_rasp,debugger system-tests: runs-on: ${{ contains(fromJSON('["CROSSED_TRACING_LIBRARIES", "INTEGRATIONS"]'), matrix.scenario) && 'ubuntu-latest-16-cores' || 'ubuntu-latest' }} diff --git a/benchmark/sirun/Dockerfile b/benchmark/sirun/Dockerfile index ad27d5d71b1..d033dc738ea 100644 --- a/benchmark/sirun/Dockerfile +++ b/benchmark/sirun/Dockerfile @@ -30,13 +30,11 @@ RUN wget -O sirun.tar.gz https://github.com/DataDog/sirun/releases/download/v0.1 RUN mkdir -p /usr/local/nvm \ && wget -q -O - https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.3/install.sh | bash \ && . $NVM_DIR/nvm.sh \ - && nvm install --no-progress 14.21.3 \ - && nvm install --no-progress 16.20.1 \ && nvm install --no-progress 18.16.1 \ && nvm install --no-progress 20.4.0 \ && nvm install --no-progress 22.10.0 \ - && nvm alias default 18 \ - && nvm use 18 + && nvm alias default 22 \ + && nvm use 22 RUN mkdir /opt/insecure-bank-js RUN git clone --depth 1 https://github.com/hdiv/insecure-bank-js.git /opt/insecure-bank-js diff --git a/ci/init.js b/ci/init.js index 7b15ed15151..dde54499e5b 100644 --- a/ci/init.js +++ b/ci/init.js @@ -1,6 +1,6 @@ /* eslint-disable no-console */ const tracer = require('../packages/dd-trace') -const { isTrue } = require('../packages/dd-trace/src/util') +const { isTrue, isFalse } = require('../packages/dd-trace/src/util') const log = require('../packages/dd-trace/src/log') const isJestWorker = !!process.env.JEST_WORKER_ID @@ -23,7 +23,7 @@ const options = { flushInterval: isJestWorker ? 0 : 5000 } -let shouldInit = true +let shouldInit = !isFalse(process.env.DD_CIVISIBILITY_ENABLED) if (isPackageManager()) { log.debug('dd-trace is not initialized in a package manager.') diff --git a/integration-tests/pino/index.js b/integration-tests/pino/index.js index 40e35388fac..2c0ec7dfe9a 100644 --- a/integration-tests/pino/index.js +++ b/integration-tests/pino/index.js @@ -18,7 +18,7 @@ const logger = require('pino')() const server = http .createServer((req, res) => { const span = tracer.scope().active() - const contextTraceId = span.context().toTraceId() + const contextTraceId = span.context().toTraceId(true) const contextSpanId = span.context().toSpanId() logger.info( { custom: { trace_id: contextTraceId, span_id: contextSpanId } }, diff --git a/packages/datadog-instrumentations/src/aws-sdk.js b/packages/datadog-instrumentations/src/aws-sdk.js index d6fbccb39a8..e91bca732c2 100644 --- a/packages/datadog-instrumentations/src/aws-sdk.js +++ b/packages/datadog-instrumentations/src/aws-sdk.js @@ -172,7 +172,7 @@ function getMessage (request, error, result) { function getChannelSuffix (name) { // some resource identifiers have spaces between ex: bedrock runtime - name = name.replaceAll(' ', '') + name = String(name).replaceAll(' ', '') return [ 'cloudwatchlogs', 'dynamodb', diff --git a/packages/datadog-instrumentations/src/http/client.js b/packages/datadog-instrumentations/src/http/client.js index 6ab01a34513..5afa47e5526 100644 --- a/packages/datadog-instrumentations/src/http/client.js +++ b/packages/datadog-instrumentations/src/http/client.js @@ -117,6 +117,11 @@ function patch (http, methodName) { } catch (e) { ctx.error = e errorChannel.publish(ctx) + // if the initial request failed, ctx.req will be unset, we must close the span here + // fix for: https://github.com/DataDog/dd-trace-js/issues/5016 + if (!ctx.req) { + finish() + } throw e } finally { endChannel.publish(ctx) diff --git a/packages/datadog-instrumentations/src/openai.js b/packages/datadog-instrumentations/src/openai.js index e41db136854..401a231f5c7 100644 --- a/packages/datadog-instrumentations/src/openai.js +++ b/packages/datadog-instrumentations/src/openai.js @@ -306,44 +306,33 @@ for (const shim of V4_PACKAGE_SHIMS) { return ch.start.runStores(ctx, () => { const apiProm = methodFn.apply(this, arguments) + if (baseResource === 'chat.completions' && typeof apiProm._thenUnwrap === 'function') { + // this should only ever be invoked from a client.beta.chat.completions.parse call + shimmer.wrap(apiProm, '_thenUnwrap', origApiPromThenUnwrap => function () { + // TODO(sam.brenner): I wonder if we can patch the APIPromise prototype instead, although + // we might not have access to everything we need... + + // this is a new apipromise instance + const unwrappedPromise = origApiPromThenUnwrap.apply(this, arguments) + + shimmer.wrap(unwrappedPromise, 'parse', origApiPromParse => function () { + const parsedPromise = origApiPromParse.apply(this, arguments) + .then(body => Promise.all([this.responsePromise, body])) + + return handleUnwrappedAPIPromise(parsedPromise, ctx, stream, n) + }) + + return unwrappedPromise + }) + } + // wrapping `parse` avoids problematic wrapping of `then` when trying to call // `withResponse` in userland code after. This way, we can return the whole `APIPromise` shimmer.wrap(apiProm, 'parse', origApiPromParse => function () { - return origApiPromParse.apply(this, arguments) - // the original response is wrapped in a promise, so we need to unwrap it + const parsedPromise = origApiPromParse.apply(this, arguments) .then(body => Promise.all([this.responsePromise, body])) - .then(([{ response, options }, body]) => { - if (stream) { - if (body.iterator) { - shimmer.wrap(body, 'iterator', wrapStreamIterator(response, options, n, ctx)) - } else { - shimmer.wrap( - body.response.body, Symbol.asyncIterator, wrapStreamIterator(response, options, n, ctx) - ) - } - } else { - finish(ctx, { - headers: response.headers, - data: body, - request: { - path: response.url, - method: options.method - } - }) - } - return body - }) - .catch(error => { - finish(ctx, undefined, error) - - throw error - }) - .finally(() => { - // maybe we don't want to unwrap here in case the promise is re-used? - // other hand: we want to avoid resource leakage - shimmer.unwrap(apiProm, 'parse') - }) + return handleUnwrappedAPIPromise(parsedPromise, ctx, stream, n) }) ch.end.publish(ctx) @@ -356,6 +345,37 @@ for (const shim of V4_PACKAGE_SHIMS) { }) } +function handleUnwrappedAPIPromise (apiProm, ctx, stream, n) { + return apiProm + .then(([{ response, options }, body]) => { + if (stream) { + if (body.iterator) { + shimmer.wrap(body, 'iterator', wrapStreamIterator(response, options, n, ctx)) + } else { + shimmer.wrap( + body.response.body, Symbol.asyncIterator, wrapStreamIterator(response, options, n, ctx) + ) + } + } else { + finish(ctx, { + headers: response.headers, + data: body, + request: { + path: response.url, + method: options.method + } + }) + } + + return body + }) + .catch(error => { + finish(ctx, undefined, error) + + throw error + }) +} + function finish (ctx, response, error) { if (error) { ctx.error = error diff --git a/packages/datadog-plugin-bunyan/test/index.spec.js b/packages/datadog-plugin-bunyan/test/index.spec.js index 036a7656449..a782958700d 100644 --- a/packages/datadog-plugin-bunyan/test/index.spec.js +++ b/packages/datadog-plugin-bunyan/test/index.spec.js @@ -72,7 +72,7 @@ describe('Plugin', () => { const record = JSON.parse(stream.write.firstCall.args[0].toString()) expect(record.dd).to.deep.include({ - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() }) }) diff --git a/packages/datadog-plugin-http/src/client.js b/packages/datadog-plugin-http/src/client.js index bf1e416e62f..94725ed95c8 100644 --- a/packages/datadog-plugin-http/src/client.js +++ b/packages/datadog-plugin-http/src/client.js @@ -102,7 +102,9 @@ class HttpClientPlugin extends ClientPlugin { addResponseHeaders(res, span, this.config) } - addRequestHeaders(req, span, this.config) + if (req) { + addRequestHeaders(req, span, this.config) + } this.config.hooks.request(span, req, res) diff --git a/packages/datadog-plugin-http/test/client.spec.js b/packages/datadog-plugin-http/test/client.spec.js index 73b2b949f62..6af6011b872 100644 --- a/packages/datadog-plugin-http/test/client.spec.js +++ b/packages/datadog-plugin-http/test/client.spec.js @@ -934,6 +934,24 @@ describe('Plugin', () => { }) }) } + + it('should record unfinished http requests as error spans', done => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('error', 1) + expect(traces[0][0].meta).to.not.have.property('http.status_code') + }) + .then(done) + .catch(done) + + try { + http.request('http://httpbin.org/get', { headers: { BadHeader: 'a\nb' } }, res => { + res.on('data', () => { }) + }) + } catch { + // expected to throw error + } + }) }) describe('with late plugin initialization and an external subscriber', () => { diff --git a/packages/datadog-plugin-mongodb-core/src/index.js b/packages/datadog-plugin-mongodb-core/src/index.js index a60182458e1..52cab5e8944 100644 --- a/packages/datadog-plugin-mongodb-core/src/index.js +++ b/packages/datadog-plugin-mongodb-core/src/index.js @@ -70,6 +70,7 @@ function getQuery (cmd) { if (!cmd || typeof cmd !== 'object' || Array.isArray(cmd)) return if (cmd.query) return sanitizeBigInt(limitDepth(cmd.query)) if (cmd.filter) return sanitizeBigInt(limitDepth(cmd.filter)) + if (cmd.pipeline) return sanitizeBigInt(limitDepth(cmd.pipeline)) } function getResource (plugin, ns, query, operationName) { diff --git a/packages/datadog-plugin-mongodb-core/test/mongodb.spec.js b/packages/datadog-plugin-mongodb-core/test/mongodb.spec.js index db6ee8ffeec..cca4ea9a2c4 100644 --- a/packages/datadog-plugin-mongodb-core/test/mongodb.spec.js +++ b/packages/datadog-plugin-mongodb-core/test/mongodb.spec.js @@ -239,6 +239,25 @@ describe('Plugin', () => { }).toArray() }) + it('should log the aggregate pipeline in mongodb.query', done => { + agent + .use(traces => { + const span = traces[0][0] + const resource = 'aggregate test.$cmd' + const query = '[{"$match":{"_id":"1234"}},{"$project":{"_id":1}}]' + + expect(span).to.have.property('resource', resource) + expect(span.meta).to.have.property('mongodb.query', query) + }) + .then(done) + .catch(done) + + collection.aggregate([ + { $match: { _id: '1234' } }, + { $project: { _id: 1 } } + ]).toArray() + }) + it('should use the toJSON method of objects if it exists', done => { const id = '123456781234567812345678' diff --git a/packages/datadog-plugin-openai/test/index.spec.js b/packages/datadog-plugin-openai/test/index.spec.js index 03ac66fb2e5..92a1f360cd4 100644 --- a/packages/datadog-plugin-openai/test/index.spec.js +++ b/packages/datadog-plugin-openai/test/index.spec.js @@ -2821,7 +2821,10 @@ describe('Plugin', () => { } if (semver.satisfies(realVersion, '>=4.0.0')) { - const result = await openai.chat.completions.create(params) + const prom = openai.chat.completions.create(params) + expect(prom).to.have.property('withResponse') + + const result = await prom expect(result.id).to.eql('chatcmpl-7GaWqyMTD9BLmkmy8SxyjUGX3KSRN') expect(result.model).to.eql('gpt-3.5-turbo-0301') @@ -3786,6 +3789,55 @@ describe('Plugin', () => { } }) } + + if (semver.intersects('>=4.59.0', version)) { + it('makes a successful call with the beta chat completions', async () => { + nock('https://api.openai.com:443') + .post('/v1/chat/completions') + .reply(200, { + id: 'chatcmpl-7GaWqyMTD9BLmkmy8SxyjUGX3KSRN', + object: 'chat.completion', + created: 1684188020, + model: 'gpt-4o', + usage: { + prompt_tokens: 37, + completion_tokens: 10, + total_tokens: 47 + }, + choices: [ + { + message: { + role: 'assistant', + content: 'I am doing well, how about you?' + }, + finish_reason: 'stop', + index: 0 + } + ] + }) + + const checkTraces = agent + .use(traces => { + const span = traces[0][0] + expect(span).to.have.property('name', 'openai.request') + }) + + const prom = openai.beta.chat.completions.parse({ + model: 'gpt-4o', + messages: [{ role: 'user', content: 'Hello, OpenAI!', name: 'hunter2' }], + temperature: 0.5, + stream: false + }) + + expect(prom).to.have.property('withResponse') + + const response = await prom + + expect(response.choices[0].message.content).to.eql('I am doing well, how about you?') + + await checkTraces + }) + } }) }) }) diff --git a/packages/datadog-plugin-pino/test/index.spec.js b/packages/datadog-plugin-pino/test/index.spec.js index 913885c8fca..5cc681605cf 100644 --- a/packages/datadog-plugin-pino/test/index.spec.js +++ b/packages/datadog-plugin-pino/test/index.spec.js @@ -110,7 +110,7 @@ describe('Plugin', () => { const record = JSON.parse(stream.write.firstCall.args[0].toString()) expect(record.dd).to.deep.include({ - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() }) @@ -184,7 +184,7 @@ describe('Plugin', () => { const record = JSON.parse(stream.write.firstCall.args[0].toString()) expect(record.dd).to.deep.include({ - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() }) @@ -207,7 +207,7 @@ describe('Plugin', () => { const record = stream.write.firstCall.args[0].toString() - expect(record).to.match(new RegExp(`trace_id\\W+?${span.context().toTraceId()}`)) + expect(record).to.match(new RegExp(`trace_id\\W+?${span.context().toTraceId(true)}`)) expect(record).to.match(new RegExp(`span_id\\W+?${span.context().toSpanId()}`)) expect(record).to.include('message') diff --git a/packages/datadog-plugin-winston/test/index.spec.js b/packages/datadog-plugin-winston/test/index.spec.js index 9033f0668cc..274a1dfbfe3 100644 --- a/packages/datadog-plugin-winston/test/index.spec.js +++ b/packages/datadog-plugin-winston/test/index.spec.js @@ -122,7 +122,7 @@ describe('Plugin', () => { it('should not alter the default behavior', () => { const meta = { dd: { - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() } } @@ -150,7 +150,7 @@ describe('Plugin', () => { it('should add the trace identifiers to the default logger', async () => { const meta = { dd: { - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() } } @@ -170,7 +170,7 @@ describe('Plugin', () => { const meta = { dd: { - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() } } @@ -190,7 +190,7 @@ describe('Plugin', () => { it('should support errors', async () => { const meta = { dd: { - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() } } @@ -216,7 +216,7 @@ describe('Plugin', () => { transports: [transport, httpTransport] }) const dd = { - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() } @@ -287,7 +287,7 @@ describe('Plugin', () => { const meta = { dd: { - trace_id: span.context().toTraceId(), + trace_id: span.context().toTraceId(true), span_id: span.context().toSpanId() } } diff --git a/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js b/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js index 8e0715a2d5d..b6715f5b6a1 100644 --- a/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js +++ b/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js @@ -69,8 +69,12 @@ class TestVisDynamicInstrumentation { // To avoid infinite initialization loops, we're disabling DI and tracing in the worker. env: { ...process.env, + DD_CIVISIBILITY_ENABLED: 0, DD_TRACE_ENABLED: 0, - DD_TEST_FAILED_TEST_REPLAY_ENABLED: 0 + DD_TEST_FAILED_TEST_REPLAY_ENABLED: 0, + DD_CIVISIBILITY_MANUAL_API_ENABLED: 0, + DD_TRACING_ENABLED: 0, + DD_TRACE_TELEMETRY_ENABLED: 0 }, workerData: { config: config.serialize(), diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 31278a0fca4..2a8dd621004 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -566,7 +566,7 @@ class Config { this._setValue(defaults, 'telemetry.metrics', true) this._setValue(defaults, 'traceEnabled', true) this._setValue(defaults, 'traceId128BitGenerationEnabled', true) - this._setValue(defaults, 'traceId128BitLoggingEnabled', false) + this._setValue(defaults, 'traceId128BitLoggingEnabled', true) this._setValue(defaults, 'tracePropagationExtractFirst', false) this._setValue(defaults, 'tracePropagationStyle.inject', ['datadog', 'tracecontext', 'baggage']) this._setValue(defaults, 'tracePropagationStyle.extract', ['datadog', 'tracecontext', 'baggage']) diff --git a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js index 229abf42df6..a13c32e2c10 100644 --- a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js +++ b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js @@ -31,9 +31,9 @@ async function addBreakpoint (probe) { probe.nsBetweenSampling = BigInt(1 / snapshotsPerSecond * 1e9) probe.lastCaptureNs = 0n - // TODO: Inbetween `await session.post('Debugger.enable')` and here, the scripts are parsed and cached. - // Maybe there's a race condition here or maybe we're guraenteed that `await session.post('Debugger.enable')` will - // not continue untill all scripts have been parsed? + // Warning: The code below relies on undocumented behavior of the inspector! + // It expects that `await session.post('Debugger.enable')` will wait for all loaded scripts to be emitted as + // `Debugger.scriptParsed` events. If this ever changes, we will have a race condition! const script = findScriptFromPartialPath(file) if (!script) throw new Error(`No loaded script found for ${file} (probe: ${probe.id}, version: ${probe.version})`) const { url, scriptId, sourceMapURL, source } = script diff --git a/packages/dd-trace/src/debugger/devtools_client/source-maps.js b/packages/dd-trace/src/debugger/devtools_client/source-maps.js index 36e12f3e5bd..203d2667019 100644 --- a/packages/dd-trace/src/debugger/devtools_client/source-maps.js +++ b/packages/dd-trace/src/debugger/devtools_client/source-maps.js @@ -7,6 +7,7 @@ const { SourceMapConsumer } = require('source-map') const cache = new Map() let cacheTimer = null +let cacheTimerLastSet = 0 const self = module.exports = { async loadSourceMap (dir, url) { @@ -33,13 +34,26 @@ const self = module.exports = { } } +// TODO: Remove if-statement around `setTimeout` below once it's safe to do so. +// +// This is a workaround for, what seems like a bug in Node.js core, that seems to trigger when, among other things, a +// lot of timers are being created very rapidly. This makes the call to `setTimeout` throw an error from within +// `AsyncLocalStorage._propagate` with the following error message: +// +// TypeError: Cannot read properties of undefined (reading 'Symbol(kResourceStore)') +// +// Source: https://github.com/nodejs/node/blob/v18.20.6/lib/async_hooks.js#L312 function cacheIt (key, value) { - clearTimeout(cacheTimer) - cacheTimer = setTimeout(function () { - // Optimize for app boot, where a lot of reads might happen - // Clear cache a few seconds after it was last used - cache.clear() - }, 10_000).unref() + const now = Date.now() + if (now > cacheTimerLastSet + 1_000) { + clearTimeout(cacheTimer) + cacheTimer = setTimeout(function () { + // Optimize for app boot, where a lot of reads might happen + // Clear cache a few seconds after it was last used + cache.clear() + }, 10_000).unref() + cacheTimerLastSet = now + } cache.set(key, value) return value } diff --git a/packages/dd-trace/src/opentracing/propagation/log.js b/packages/dd-trace/src/opentracing/propagation/log.js index 2203f253fb6..fb0bcbf531f 100644 --- a/packages/dd-trace/src/opentracing/propagation/log.js +++ b/packages/dd-trace/src/opentracing/propagation/log.js @@ -14,7 +14,8 @@ class LogPropagator { carrier.dd = {} if (spanContext) { - if (this._config.traceId128BitLoggingEnabled && spanContext._trace.tags['_dd.p.tid']) { + if (this._config.traceId128BitGenerationEnabled && + this._config.traceId128BitLoggingEnabled && spanContext._trace.tags['_dd.p.tid']) { carrier.dd.trace_id = spanContext.toTraceId(true) } else { carrier.dd.trace_id = spanContext.toTraceId() diff --git a/packages/dd-trace/src/opentracing/propagation/text_map.js b/packages/dd-trace/src/opentracing/propagation/text_map.js index fd7d32760cb..956d2c0aa8f 100644 --- a/packages/dd-trace/src/opentracing/propagation/text_map.js +++ b/packages/dd-trace/src/opentracing/propagation/text_map.js @@ -46,6 +46,7 @@ const tracestateTagKeyFilter = /[^\x21-\x2b\x2d-\x3c\x3e-\x7e]/g const tracestateTagValueFilter = /[^\x20-\x2b\x2d-\x3a\x3c-\x7d]/g const invalidSegment = /^0+$/ const zeroTraceId = '0000000000000000' +const hex16 = /^[0-9A-Fa-f]{16}$/ class TextMapPropagator { constructor (config) { @@ -482,10 +483,20 @@ class TextMapPropagator { } break } - default: + default: { if (!key.startsWith('t.')) continue - spanContext._trace.tags[`_dd.p.${key.slice(2)}`] = value - .replace(/[\x7e]/gm, '=') + const subKey = key.slice(2) // e.g. t.tid -> tid + const transformedValue = value.replace(/[\x7e]/gm, '=') + + // If subkey is tid then do nothing because trace header tid should always be preserved + if (subKey === 'tid') { + if (!hex16.test(value) || spanContext._trace.tags['_dd.p.tid'] !== transformedValue) { + log.error(`Invalid trace id ${value} in tracestate, skipping`) + } + continue + } + spanContext._trace.tags[`_dd.p.${subKey}`] = transformedValue + } } } }) @@ -645,7 +656,11 @@ class TextMapPropagator { log.error('Trace tags from carrier are invalid, skipping extraction.') return } - + // Check if value is a valid 16 character lower-case hexadecimal encoded number as per spec + if (key === '_dd.p.tid' && !(hex16.test(value))) { + log.error(`Invalid _dd.p.tid tag ${value}, skipping`) + continue + } tags[key] = value } diff --git a/packages/dd-trace/src/telemetry/index.js b/packages/dd-trace/src/telemetry/index.js index 9328186a82a..08a6add25ec 100644 --- a/packages/dd-trace/src/telemetry/index.js +++ b/packages/dd-trace/src/telemetry/index.js @@ -327,7 +327,8 @@ function updateConfig (changes, config) { service: 'DD_SERVICE', clientIpHeader: 'DD_TRACE_CLIENT_IP_HEADER', 'grpc.client.error.statuses': 'DD_GRPC_CLIENT_ERROR_STATUSES', - 'grpc.server.error.statuses': 'DD_GRPC_SERVER_ERROR_STATUSES' + 'grpc.server.error.statuses': 'DD_GRPC_SERVER_ERROR_STATUSES', + traceId128BitLoggingEnabled: 'DD_TRACE_128_BIT_TRACEID_LOGGING_ENABLED' } const namesNeedFormatting = new Set(['DD_TAGS', 'peerServiceMapping', 'serviceMapping']) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 9cfb2db8e75..9a481e375d1 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -228,7 +228,7 @@ describe('Config', () => { expect(config).to.have.nested.deep.property('dynamicInstrumentation.redactedIdentifiers', []) expect(config).to.have.nested.deep.property('dynamicInstrumentation.redactionExcludedIdentifiers', []) expect(config).to.have.property('traceId128BitGenerationEnabled', true) - expect(config).to.have.property('traceId128BitLoggingEnabled', false) + expect(config).to.have.property('traceId128BitLoggingEnabled', true) expect(config).to.have.property('spanAttributeSchema', 'v0') expect(config.grpc.client.error.statuses).to.deep.equal(GRPC_CLIENT_ERROR_STATUSES) expect(config.grpc.server.error.statuses).to.deep.equal(GRPC_SERVER_ERROR_STATUSES) @@ -393,7 +393,7 @@ describe('Config', () => { { name: 'telemetry.logCollection', value: true, origin: 'default' }, { name: 'telemetry.metrics', value: true, origin: 'default' }, { name: 'traceId128BitGenerationEnabled', value: true, origin: 'default' }, - { name: 'traceId128BitLoggingEnabled', value: false, origin: 'default' }, + { name: 'traceId128BitLoggingEnabled', value: true, origin: 'default' }, { name: 'tracing', value: true, origin: 'default' }, { name: 'url', value: undefined, origin: 'default' }, { name: 'version', value: '', origin: 'default' } diff --git a/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js b/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js index 68cbea0986c..768c482c43e 100644 --- a/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js +++ b/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js @@ -98,7 +98,9 @@ describe('source map utils', function () { let clock function setup () { - clock = sinon.useFakeTimers() + clock = sinon.useFakeTimers({ + toFake: ['setTimeout'] + }) readFileSync = sinon.stub().returns(rawSourceMap) readFile = sinon.stub().resolves(rawSourceMap) diff --git a/packages/dd-trace/test/opentracing/propagation/log.spec.js b/packages/dd-trace/test/opentracing/propagation/log.spec.js index 50c815f1b7c..88ec0104892 100644 --- a/packages/dd-trace/test/opentracing/propagation/log.spec.js +++ b/packages/dd-trace/test/opentracing/propagation/log.spec.js @@ -58,6 +58,7 @@ describe('LogPropagator', () => { it('should inject 128-bit trace IDs when enabled', () => { config.traceId128BitLoggingEnabled = true + config.traceId128BitGenerationEnabled = true const carrier = {} const traceId = id('1234567812345678') @@ -78,6 +79,7 @@ describe('LogPropagator', () => { it('should correctly inject 128 bit trace ids when _dd.p.tid is present', () => { config.traceId128BitLoggingEnabled = true + config.traceId128BitGenerationEnabled = true const carrier = {} const traceId = id('4e2a9c1573d240b1a3b7e3c1d4c2f9a7', 16) const traceIdTag = '8765432187654321' @@ -96,6 +98,28 @@ describe('LogPropagator', () => { }) it('should not inject 128-bit trace IDs when disabled', () => { + config.traceId128BitLoggingEnabled = false + config.traceId128BitGenerationEnabled = true + const carrier = {} + const traceId = id('123', 10) + const traceIdTag = '8765432187654321' + const spanContext = new SpanContext({ + traceId, + spanId: id('456', 10) + }) + + spanContext._trace.tags['_dd.p.tid'] = traceIdTag + + propagator.inject(spanContext, carrier) + + expect(carrier).to.have.property('dd') + expect(carrier.dd).to.have.property('trace_id', '123') + expect(carrier.dd).to.have.property('span_id', '456') + }) + + it('should not inject 128-bit trace IDs when 128 bit generation is disabled', () => { + config.traceId128BitLoggingEnabled = true + config.traceId128BitGenerationEnabled = false const carrier = {} const traceId = id('123', 10) const traceIdTag = '8765432187654321' diff --git a/packages/dd-trace/test/opentracing/propagation/text_map.spec.js b/packages/dd-trace/test/opentracing/propagation/text_map.spec.js index 2c699b107a1..8199c7ed944 100644 --- a/packages/dd-trace/test/opentracing/propagation/text_map.spec.js +++ b/packages/dd-trace/test/opentracing/propagation/text_map.spec.js @@ -451,6 +451,41 @@ describe('TextMapPropagator', () => { expect(spanContextD._baggageItems).to.deep.equal({}) }) + it('should discard malformed tids', () => { + // tid with malformed characters + let carrier = { + 'x-datadog-trace-id': '1234567890123456789', + 'x-datadog-parent-id': '987654321', + 'x-datadog-tags': '_dd.p.tid=1234567890abcdeX' + } + let spanContext = propagator.extract(carrier) + expect(spanContext.toTraceId()).to.equal(carrier['x-datadog-trace-id']) + expect(spanContext.toSpanId()).to.equal(carrier['x-datadog-parent-id']) + expect(spanContext._trace.tags).to.not.have.property('_dd.p.tid') + + // tid too long + carrier = { + 'x-datadog-trace-id': '234567890123456789', + 'x-datadog-parent-id': '987654321', + 'x-datadog-tags': '_dd.p.tid=1234567890abcdef1' + } + spanContext = propagator.extract(carrier) + expect(spanContext.toTraceId()).to.equal(carrier['x-datadog-trace-id']) + expect(spanContext.toSpanId()).to.equal(carrier['x-datadog-parent-id']) + expect(spanContext._trace.tags).to.not.have.property('_dd.p.tid') + + // tid too short + carrier = { + 'x-datadog-trace-id': '1234567890123456789', + 'x-datadog-parent-id': '987654321', + 'x-datadog-tags': '_dd.p.tid=1234567890abcde' + } + spanContext = propagator.extract(carrier) + expect(spanContext.toTraceId()).to.equal(carrier['x-datadog-trace-id']) + expect(spanContext.toSpanId()).to.equal(carrier['x-datadog-parent-id']) + expect(spanContext._trace.tags).to.not.have.property('_dd.p.tid') + }) + // temporary test. On the contrary, it SHOULD extract baggage it('should not extract baggage when it is the only propagation style', () => { config = new Config({ @@ -630,6 +665,30 @@ describe('TextMapPropagator', () => { expect(spanContext._trace.tags).to.have.property('_dd.parent_id', '2244eeee6666aaaa') }) + it('should preserve trace header tid when tracestate contains an inconsistent tid', () => { + textMap.traceparent = '00-640cfd8d00000000abcdefab12345678-000000003ade68b1-01' + textMap.tracestate = 'dd=t.tid:640cfd8d0000ffff' + config.tracePropagationStyle.extract = ['tracecontext'] + + const carrier = textMap + const spanContext = propagator.extract(carrier) + + expect(spanContext._traceId.toString(16)).to.equal('640cfd8d00000000abcdefab12345678') + expect(spanContext._trace.tags).to.have.property('_dd.p.tid', '640cfd8d00000000') + }) + + it('should preserve trace header tid when tracestate contains a malformed tid', () => { + textMap.traceparent = '00-640cfd8d00000000abcdefab12345678-000000003ade68b1-01' + textMap.tracestate = 'dd=t.tid:XXXX' + config.tracePropagationStyle.extract = ['tracecontext'] + + const carrier = textMap + const spanContext = propagator.extract(carrier) + + expect(spanContext._traceId.toString(16)).to.equal('640cfd8d00000000abcdefab12345678') + expect(spanContext._trace.tags).to.have.property('_dd.p.tid', '640cfd8d00000000') + }) + it('should set the last datadog parent id to zero when p: is NOT in the tracestate', () => { textMap.traceparent = '00-0000000000000000000000000000007B-0000000000000456-01' textMap.tracestate = 'other=gg,dd=s:-1;' diff --git a/packages/dd-trace/test/plugins/log_plugin.spec.js b/packages/dd-trace/test/plugins/log_plugin.spec.js index 0a9c1599167..609c5ff8565 100644 --- a/packages/dd-trace/test/plugins/log_plugin.spec.js +++ b/packages/dd-trace/test/plugins/log_plugin.spec.js @@ -61,7 +61,7 @@ describe('LogPlugin', () => { expect(message.dd).to.contain(config) // Should have trace/span data when none is active - expect(message.dd).to.have.property('trace_id', span.context().toTraceId()) + expect(message.dd).to.have.property('trace_id', span.context().toTraceId(true)) expect(message.dd).to.have.property('span_id', span.context().toSpanId()) }) }) diff --git a/scripts/verify-ci-config.js b/scripts/verify-ci-config.js index becc7287487..25b642ed8cd 100644 --- a/scripts/verify-ci-config.js +++ b/scripts/verify-ci-config.js @@ -159,9 +159,31 @@ checkPlugins(path.join(__dirname, '..', '.github', 'workflows', 'appsec.yml')) /// / Verifying that tests run on correct triggers /// / +const IGNORED_WORKFLOWS = { + all: [ + 'prepare-release-proposal.yml', + 'rebase-release-proposal.yml', + 'release-3.yml', + 'release-4.yml', + 'release-dev.yml', + 'release-latest.yml', + 'release-proposal.yml', + 'codeql-analysis.yml', + 'pr-labels.yml' + ], + trigger_pull_request: [ + 'stale.yml' + ], + trigger_push: [ + 'package-size.yml', + 'stale.yml' + ], + trigger_schedule: [] +} + const workflows = fs.readdirSync(path.join(__dirname, '..', '.github', 'workflows')) .filter(file => - !['release', 'codeql', 'pr-labels'] + !IGNORED_WORKFLOWS.all .reduce((contained, name) => contained || file.includes(name), false) ) @@ -173,13 +195,16 @@ for (const workflow of workflows) { const yamlPath = path.join(__dirname, '..', '.github', 'workflows', workflow) const yamlContent = yaml.parse(fs.readFileSync(yamlPath, 'utf8')) const triggers = yamlContent.on - if (triggers?.pull_request !== null) { + if (!IGNORED_WORKFLOWS.trigger_pull_request.includes(workflow) && + triggers?.pull_request !== null) { triggersError(workflow, 'The `pull_request` trigger should be blank') } - if (workflow !== 'package-size.yml' && triggers?.push?.branches?.[0] !== 'master') { + if (!IGNORED_WORKFLOWS.trigger_push.includes(workflow) && + triggers?.push?.branches?.[0] !== 'master') { triggersError(workflow, 'The `push` trigger should run on master') } - if (triggers?.schedule?.[0]?.cron !== '0 4 * * *') { + if (!IGNORED_WORKFLOWS.trigger_schedule.includes(workflow) && + triggers?.schedule?.[0]?.cron !== '0 4 * * *') { triggersError(workflow, 'The `cron` trigger should be \'0 4 * * *\'') } }