From df966699b9f5d0329ea30bab68ed75f65cf9989c Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Thu, 3 Sep 2026 17:11:00 +0000 Subject: [PATCH] feat(debugger): report condition evaluation errors A probe whose condition threw was silently treated as not matching, so a broken condition (a typo in a variable name, a member access on an undefined value, ...) left the user with a probe that never fires and no explanation. The condition was also evaluated again, and threw again, on every single hit. Condition errors are now handled like the other tracers do it: the probe result carries the error in `evaluationErrors` (with the condition DSL as `expr` and the error as `message`) and nothing is captured, so the user can see why the probe is not producing regular results. Error results bypass the per-probe and global rate limits, and are instead limited by a per-probe throttle: after an error the condition is not evaluated again for five minutes, and one error result is reported per window. This also bounds the cost of a condition that is expensive to fail. The probe sampler records the error on the main thread and flags the sampled probe index, so the worker knows to fetch the error from the paused frame instead of capturing a snapshot. --- integration-tests/debugger/conditions.spec.js | 33 ++++ .../debugger/diagnostics.spec.js | 13 +- .../src/debugger/devtools_client/index.js | 44 +++++- .../debugger/devtools_client/probe_sampler.js | 26 +++- .../dd-trace/src/debugger/probe_sampler.js | 89 ++++++++++- .../src/debugger/probe_sampler_constants.js | 9 ++ .../debugger/devtools_client/index.spec.js | 104 ++++++++++++- .../test/debugger/probe_sampler.spec.js | 142 +++++++++++++++++- 8 files changed, 432 insertions(+), 28 deletions(-) diff --git a/integration-tests/debugger/conditions.spec.js b/integration-tests/debugger/conditions.spec.js index 0e7fbc5f93c..c42bf187407 100644 --- a/integration-tests/debugger/conditions.spec.js +++ b/integration-tests/debugger/conditions.spec.js @@ -45,6 +45,39 @@ describe('Dynamic Instrumentation', function () { assert.deepStrictEqual(snapshots, []) }) + it('should report an error result if the condition throws, once per throttle window', async function () { + const rcConfig = t.generateRemoteConfig({ + captureSnapshot: true, + when: { dsl: 'definitelyDoesNotExist == "never"', json: { eq: [{ ref: 'definitelyDoesNotExist' }, 'never'] } }, + }) + const probeInstalled = t.waitForProbeStatus([rcConfig.config.id], 'INSTALLED') + + t.agent.addRemoteConfig(rcConfig) + await probeInstalled + + const resultReceived = new Promise(resolve => { + t.agent.once('debugger-input', ({ payload }) => resolve(payload[0])) + }) + const [snapshots, result] = await Promise.all([ + t.captureSnapshotsUntilExit(1, async () => { + await t.request(t.breakpoint.url) + await Promise.all([t.request(t.breakpoint.url), t.request(t.breakpoint.url)]) + await delay(1500) + }), + resultReceived, + ]) + + assert.strictEqual(snapshots.length, 1, 'should only report the condition error once') + assert.strictEqual(result.message, 'ReferenceError: definitelyDoesNotExist is not defined') + const [snapshot] = snapshots + assert.deepStrictEqual(snapshot.evaluationErrors, [{ + expr: 'definitelyDoesNotExist == "never"', + message: 'ReferenceError: definitelyDoesNotExist is not defined', + }]) + assert.strictEqual(snapshot.captures, undefined, 'should not capture anything for a failing condition') + assert.strictEqual(snapshot.probe.id, rcConfig.config.id) + }) + it('should report error if condition cannot be compiled', async function () { const rcConfig = t.generateRemoteConfig({ when: { dsl: 'original dsl', json: { ref: 'this is not a valid ref' } }, diff --git a/integration-tests/debugger/diagnostics.spec.js b/integration-tests/debugger/diagnostics.spec.js index 9e29e29118d..ba407012a80 100644 --- a/integration-tests/debugger/diagnostics.spec.js +++ b/integration-tests/debugger/diagnostics.spec.js @@ -379,8 +379,12 @@ describe('Dynamic Instrumentation', function () { it('should support not triggering any probes when all conditions are not met', async function () { const configs = [ - t.generateRemoteConfig({ when: { json: { eq: [{ ref: 'foo' }, 'bar'] } } }), - t.generateRemoteConfig({ when: { json: { eq: [{ ref: 'foo' }, 'baz'] } } }), + t.generateRemoteConfig({ + when: { json: { eq: [{ getmember: [{ getmember: [{ ref: 'request' }, 'params'] }, 'name'] }, 'invalid'] } }, + }), + t.generateRemoteConfig({ + when: { json: { eq: [{ getmember: [{ getmember: [{ ref: 'request' }, 'params'] }, 'name'] }, 'nope'] } }, + }), ] const { emittingProbeIds, response } = await captureEmittingProbesUntilExit(t, configs, []) @@ -406,16 +410,17 @@ describe('Dynamic Instrumentation', function () { it('trigger on met condition, even if other condition throws (all have conditions)', async function () { const configs = [ + // This condition throws because `foo` is not defined, which is reported as an error result. t.generateRemoteConfig({ when: { json: { eq: [{ ref: 'foo' }, 'bar'] } } }), t.generateRemoteConfig({ when: { json: { eq: [{ getmember: [{ getmember: [{ ref: 'request' }, 'params'] }, 'name'] }, 'bar'] } }, }), ] - const expectedProbeIds = [configs[1].config.id] + const expectedProbeIds = configs.map(config => config.config.id) const { emittingProbeIds, response } = await captureEmittingProbesUntilExit(t, configs, expectedProbeIds) assert.strictEqual(response.status, 200) - assert.deepStrictEqual(emittingProbeIds, expectedProbeIds) + assert.deepStrictEqual(emittingProbeIds.sort(), expectedProbeIds.sort()) }) it('should only trigger the probes whose conditions are met (not all have conditions)', async function () { diff --git a/packages/dd-trace/src/debugger/devtools_client/index.js b/packages/dd-trace/src/debugger/devtools_client/index.js index f2dcc30241c..428dd801a3e 100644 --- a/packages/dd-trace/src/debugger/devtools_client/index.js +++ b/packages/dd-trace/src/debugger/devtools_client/index.js @@ -7,11 +7,13 @@ const processTags = require('../../process-tags') const { INSPECT_SEGMENT_GLOBAL_PROPERTY } = require('../constants') const { EVENT_TYPE, INCOMPLETE_REASON } = require('../guardrail-metrics') const { + CONDITION_ERROR_FLAG, MAX_SAMPLED_PROBES_PER_PAUSE, SAMPLED_PROBE_COUNT_INDEX, SAMPLED_PROBE_INDEXES_START, SAMPLED_PROBE_OVERFLOW_INDEX, } = require('../probe_sampler_constants') +const { getTakeConditionErrorExpression } = require('./probe_sampler') const { breakpointToProbes, samplingIndexToProbe } = require('./state') const { refreshBreakpoint } = require('./breakpoints') const session = require('./session') @@ -57,7 +59,11 @@ session.on('Debugger.paused', async ({ params }) => { let numberOfProbesWithSnapshots = 0 let probesWithCaptureExpressions = false const probes = [] - let templateExpressions = '' + // Expressions evaluated on the paused frame in one round trip, in the order of `probes`: the evaluated template for + // probes whose template requires evaluation, and the recorded error for probes paused to report a condition error + let frameExpressions = '' + /** @type {Set | undefined} */ + let conditionErrorProbes // V8 doesn't allow setting more than one breakpoint at a specific location, however, it's possible to set two // breakpoints just next to each other that will "snap" to the same logical location, which in turn will be hit at the @@ -83,7 +89,8 @@ session.on('Debugger.paused', async ({ params }) => { } for (let j = 0; j < numberOfSampledProbeIndexes; j++) { - const samplingIndex = Atomics.load(sampledProbeIndexes, SAMPLED_PROBE_INDEXES_START + j) + const sampledValue = Atomics.load(sampledProbeIndexes, SAMPLED_PROBE_INDEXES_START + j) + const samplingIndex = sampledValue & ~CONDITION_ERROR_FLAG const probe = samplingIndexToProbe.get(samplingIndex) if (probe === undefined) { @@ -96,6 +103,15 @@ session.on('Debugger.paused', async ({ params }) => { continue } + if ((sampledValue & CONDITION_ERROR_FLAG) !== 0) { + // The condition threw, so there's nothing to capture. Only the recorded error is needed from the paused thread. + conditionErrorProbes ??= new Set() + conditionErrorProbes.add(probe) + frameExpressions += `,${getTakeConditionErrorExpression(probe.id)}` + probes.push(probe) + continue + } + if (probe.captureSnapshot === true || probe.compiledCaptureExpressions !== undefined) { if (probe.captureSnapshot === true) { numberOfProbesWithSnapshots++ @@ -109,7 +125,7 @@ session.on('Debugger.paused', async ({ params }) => { } if (probe.templateRequiresEvaluation) { - templateExpressions += `,${probe.template}` + frameExpressions += `,${probe.template}` } probes.push(probe) @@ -127,9 +143,9 @@ session.on('Debugger.paused', async ({ params }) => { const { result } = /** @type {EvaluateOnCallFrameResult} */ ( await session.post('Debugger.evaluateOnCallFrame', { callFrameId: params.callFrames[0].callFrameId, - expression: templateExpressions.length === 0 + expression: frameExpressions.length === 0 ? `[${getDDTagsExpression}]` - : `${templateExpressionSetupCode}[${getDDTagsExpression}${templateExpressions}]`, + : `${templateExpressionSetupCode}[${getDDTagsExpression}${frameExpressions}]`, returnByValue: true, includeCommandLineAPI: true, }) @@ -188,7 +204,7 @@ session.on('Debugger.paused', async ({ params }) => { } const stack = await getStackFromCallFrames(params.callFrames) - const dd = processDD(evalResults[0]) // the first result is the dd tags, the rest are the probe template results + const dd = processDD(evalResults[0]) // the first result is the dd tags, the rest are the frame expression results let messageIndex = 1 // A probe whose capture got permanently disabled during this pause, if any @@ -215,6 +231,22 @@ session.on('Debugger.paused', async ({ params }) => { let eventType = EVENT_TYPE.LOG let incompleteReasons = 0 + if (conditionErrorProbes?.has(probe)) { + // Report the failing condition instead of a probe result, so the user can see why the probe doesn't fire + const error = evalResults[messageIndex++] + const message = typeof error === 'string' ? error : 'Unknown evaluation error' + log.debug('[debugger:devtools_client] Condition of probe %s failed to evaluate: %s', probe.id, message) + snapshot.evaluationErrors = [{ expr: probe.when.dsl, message }] + ackEmitting(probe) + send(message, logger, dd, snapshot, + config.propagateProcessTags.enabled ? processTags.serialized : undefined, + probe.captureSnapshot === true || probe.compiledCaptureExpressions !== undefined + ? EVENT_TYPE.SNAPSHOT + : EVENT_TYPE.LOG, + 0) + continue + } + if (probe.captureSnapshot) { eventType = EVENT_TYPE.SNAPSHOT const { processLocalState, fatalErrors, incomplete } = /** @type {NonNullable} */ (localState) diff --git a/packages/dd-trace/src/debugger/devtools_client/probe_sampler.js b/packages/dd-trace/src/debugger/devtools_client/probe_sampler.js index c30a8822043..3eee7ea67d9 100644 --- a/packages/dd-trace/src/debugger/devtools_client/probe_sampler.js +++ b/packages/dd-trace/src/debugger/devtools_client/probe_sampler.js @@ -8,6 +8,7 @@ const SAMPLER_EXPRESSION = `globalThis[Symbol.for(${JSON.stringify(DD_TRACE_SYMB module.exports = { compileBreakpointCondition, getRemoveProbeExpression, + getTakeConditionErrorExpression, isSnapshotProducingProbe, } @@ -34,6 +35,17 @@ function getRemoveProbeExpression (id) { return `${SAMPLER_EXPRESSION}?.remove(${JSON.stringify(id)})` } +/** + * Build the expression that hands over the condition error recorded for a probe. Called by the devtools worker and + * evaluated on the paused frame of the debuggee. + * + * @param {string} id - The probe id. + * @returns {string} + */ +function getTakeConditionErrorExpression (id) { + return `${SAMPLER_EXPRESSION}?.takeConditionError(${JSON.stringify(id)})` +} + /** * Build a Chrome DevTools breakpoint condition that samples all matching probes at a location. Called by the devtools * worker. @@ -85,9 +97,15 @@ function compileProbeCondition (probe) { return `$dd_sampled = ${sample} || $dd_sampled` } - return `try { - if ((${probe.condition}) === true) { - $dd_sampled = ${sample} || $dd_sampled + // A condition that throws is reported once per throttle window and skipped at probe entry in between + return `if ($dd_sampler.shouldEvaluateCondition(${JSON.stringify(probe.id)})) { + try { + if ((${probe.condition}) === true) { + $dd_sampled = ${sample} || $dd_sampled + } + } catch ($dd_error) { + $dd_sampled = $dd_sampler.conditionError(${probe.samplingIndex}, ${JSON.stringify(probe.id)}, $dd_error) || + $dd_sampled } - } catch {}` + }` } diff --git a/packages/dd-trace/src/debugger/probe_sampler.js b/packages/dd-trace/src/debugger/probe_sampler.js index b9b153da33d..6eec3acdc18 100644 --- a/packages/dd-trace/src/debugger/probe_sampler.js +++ b/packages/dd-trace/src/debugger/probe_sampler.js @@ -3,6 +3,8 @@ const { MAX_SNAPSHOTS_PER_SECOND_GLOBALLY } = require('./devtools_client/defaults') const { EVENT_TYPE, SKIPPED_REASON } = require('./guardrail-metrics') const { + CONDITION_ERROR_FLAG, + CONDITION_ERROR_THROTTLE_NS, DD_TRACE_SYMBOL, MAX_SAMPLED_PROBES_PER_PAUSE, PROBE_SAMPLER_SYMBOL, @@ -30,6 +32,13 @@ function installProbeSampler (guardrailMetrics) { const buffer = createProbeSamplerBuffer() const lastCaptureNsByProbeId = new Map() + /** + * Probes whose condition recently failed to evaluate, keyed by probe id. The error is kept until the worker picks it + * up for the error result. + * + * @type {Map} + */ + const conditionErrorByProbeId = new Map() const sampledProbeIndexes = new Int32Array(buffer) Atomics.store(sampledProbeIndexes, SAMPLED_PROBE_COUNT_INDEX, 0) Atomics.store(sampledProbeIndexes, SAMPLED_PROBE_OVERFLOW_INDEX, 0) @@ -69,11 +78,7 @@ function installProbeSampler (guardrailMetrics) { } } - const sampledProbeCount = Atomics.add(sampledProbeIndexes, SAMPLED_PROBE_COUNT_INDEX, 1) - if (sampledProbeCount >= MAX_SAMPLED_PROBES_PER_PAUSE) { - Atomics.store(sampledProbeIndexes, SAMPLED_PROBE_OVERFLOW_INDEX, 1) - return false - } + if (!storeSampledProbeIndex(probeIndex)) return false if (isSnapshotProducingProbe === true) { if (shouldResetGlobalSnapshotRateWindow === true) { @@ -85,10 +90,53 @@ function installProbeSampler (guardrailMetrics) { } lastCaptureNsByProbeId.set(probeId, now) - Atomics.store(sampledProbeIndexes, SAMPLED_PROBE_INDEXES_START + sampledProbeCount, probeIndex) return true }, + /** + * Decide if a probe's condition should be evaluated, or skipped because a recent evaluation error throttled it. + * + * @param {string} probeId - The probe id. + * @returns {boolean} Whether the condition should be evaluated on this hit. + */ + shouldEvaluateCondition (probeId) { + const state = conditionErrorByProbeId.get(probeId) + return state === undefined || process.hrtime.bigint() >= state.throttledUntilNs + }, + + /** + * Record that a probe's condition threw, throttle the probe, and request a pause so the error can be reported. + * + * Error results bypass the per-probe and global rate limits: they are rate limited by the throttle instead, which + * allows one error result per probe per window. + * + * @param {number} probeIndex - The worker-side probe sampling index. + * @param {string} probeId - The probe id. + * @param {unknown} error - The value thrown by the condition. + * @returns {boolean} Whether this probe should make the breakpoint condition pause. + */ + conditionError (probeIndex, probeId, error) { + conditionErrorByProbeId.set(probeId, { + throttledUntilNs: process.hrtime.bigint() + CONDITION_ERROR_THROTTLE_NS, + error: describeError(error), + }) + return storeSampledProbeIndex(probeIndex | CONDITION_ERROR_FLAG) + }, + + /** + * Hand over the recorded condition error for a probe to the worker. Called by the worker on the paused thread. + * + * @param {string} probeId - The probe id. + * @returns {string | undefined} The error description, if any. + */ + takeConditionError (probeId) { + const state = conditionErrorByProbeId.get(probeId) + if (state === undefined) return + const { error } = state + state.error = undefined + return error + }, + /** * Remove cached sampling state for a probe. * @@ -96,9 +144,26 @@ function installProbeSampler (guardrailMetrics) { */ remove (probeId) { lastCaptureNsByProbeId.delete(probeId) + conditionErrorByProbeId.delete(probeId) }, } + /** + * Hand a sampled probe index over to the worker for the upcoming pause. + * + * @param {number} value - The probe sampling index, possibly with flags set. + * @returns {boolean} `false` if the shared buffer is full and the probe must be skipped. + */ + function storeSampledProbeIndex (value) { + const sampledProbeCount = Atomics.add(sampledProbeIndexes, SAMPLED_PROBE_COUNT_INDEX, 1) + if (sampledProbeCount >= MAX_SAMPLED_PROBES_PER_PAUSE) { + Atomics.store(sampledProbeIndexes, SAMPLED_PROBE_OVERFLOW_INDEX, 1) + return false + } + Atomics.store(sampledProbeIndexes, SAMPLED_PROBE_INDEXES_START + sampledProbeCount, value) + return true + } + return buffer } @@ -109,6 +174,18 @@ function uninstallProbeSampler () { delete ddTraceGlobal[Symbol.for(PROBE_SAMPLER_SYMBOL)] } +/** + * Describe a value thrown by a probe condition the way the template evaluation does, without touching the value if it's + * not an error, since conditions can throw anything. + * + * @param {unknown} error - The thrown value. + * @returns {string} + */ +function describeError (error) { + if (error instanceof Error) return `${error.name}: ${error.message}` + return typeof error === 'string' ? error : 'Unknown evaluation error' +} + /** * Create the shared buffer used to hand sampled probe indexes from breakpoint conditions to the debugger worker. * diff --git a/packages/dd-trace/src/debugger/probe_sampler_constants.js b/packages/dd-trace/src/debugger/probe_sampler_constants.js index 2b67bcdddf1..11cfce81723 100644 --- a/packages/dd-trace/src/debugger/probe_sampler_constants.js +++ b/packages/dd-trace/src/debugger/probe_sampler_constants.js @@ -9,7 +9,16 @@ const SAMPLED_PROBE_COUNT_INDEX = 0 const SAMPLED_PROBE_OVERFLOW_INDEX = 1 const SAMPLED_PROBE_INDEXES_START = 2 +// Set on a sampled probe index when the pause is for reporting a condition evaluation error instead of a probe result. +const CONDITION_ERROR_FLAG = 1 << 30 + +// A probe whose condition failed to evaluate is not evaluated again for this long. One error result is reported per +// window, so a probe with a broken condition stays visible without repeatedly paying for the failing evaluation. +const CONDITION_ERROR_THROTTLE_NS = 5n * 60n * 1_000_000_000n // 5 minutes + module.exports = { + CONDITION_ERROR_FLAG, + CONDITION_ERROR_THROTTLE_NS, DD_TRACE_SYMBOL, MAX_SAMPLED_PROBES_PER_PAUSE, PROBE_SAMPLER_SYMBOL, diff --git a/packages/dd-trace/test/debugger/devtools_client/index.spec.js b/packages/dd-trace/test/debugger/devtools_client/index.spec.js index 97e46b5c2fa..700566ef45a 100644 --- a/packages/dd-trace/test/debugger/devtools_client/index.spec.js +++ b/packages/dd-trace/test/debugger/devtools_client/index.spec.js @@ -12,7 +12,10 @@ require('../../setup/mocha') const { LARGE_OBJECT_SKIP_THRESHOLD } = require('../../../src/debugger/devtools_client/snapshot/constants') const { EVENT_TYPE, GuardrailMetrics, INCOMPLETE_REASON } = require('../../../src/debugger/guardrail-metrics') const { installProbeSampler } = require('../../../src/debugger/probe_sampler') -const { MAX_SAMPLED_PROBES_PER_PAUSE } = require('../../../src/debugger/probe_sampler_constants') +const { + CONDITION_ERROR_FLAG, + MAX_SAMPLED_PROBES_PER_PAUSE, +} = require('../../../src/debugger/probe_sampler_constants') const breakpoint = { file: 'file.js', line: 1 } const breakpointId = 'breakpoint-id' @@ -139,12 +142,13 @@ describe('onPause', function () { * Attach a probe to the hit breakpoint and mark it as sampled for the next pause. * * @param {ReturnType} probe - The probe to sample. + * @param {number} [flags] - Flags to set on the sampled probe index. */ - function sampleProbe (probe) { + function sampleProbe (probe, flags = 0) { state.breakpointToProbes.set(breakpointId, new Map([[probe.id, probe]])) state.samplingIndexToProbe.set(1, probe) Atomics.store(sampledProbeIndexes, 0, 1) - Atomics.store(sampledProbeIndexes, 2, 1) + Atomics.store(sampledProbeIndexes, 2, 1 | flags) } it('should not fail if there is no probe for at the breakpoint', async function () { @@ -449,6 +453,100 @@ describe('onPause', function () { sinon.assert.calledOnceWithExactly(refreshBreakpoint, probe) }) + describe('condition errors', function () { + const takeConditionErrorExpression = 'globalThis[Symbol.for("dd-trace")]?.' + + '[Symbol.for("dd-trace.debugger.probeSampler")]?.takeConditionError("probe-1")' + + it('should report the condition error instead of a probe result', async function () { + const probe = genProcessedProbe('probe-1') + probe.captureSnapshot = true + probe.capture = { maxReferenceDepth: 3, maxCollectionSize: 100, maxFieldCount: 20, maxLength: 255 } + probe.when = { dsl: 'foo.bar == 42', json: {} } + probe.condition = 'foo.bar === 42' + sampleProbe(probe, CONDITION_ERROR_FLAG) + + session.post = sinon.stub().callsFake((method, params) => { + if (method === 'Debugger.evaluateOnCallFrame') { + assert.strictEqual(params.expression.slice(-takeConditionErrorExpression.length - 2), + `,${takeConditionErrorExpression}]`) + return Promise.resolve({ result: { value: [{}, 'TypeError: boom'] } }) + } + return Promise.resolve({}) + }) + + await onPaused(event) + + sinon.assert.calledWith(session.post.secondCall, 'Debugger.resume') + sinon.assert.neverCalledWith(session.post, 'Runtime.getProperties') + sinon.assert.calledOnceWithExactly(ackEmitting, probe) + sinon.assert.calledOnce(send) + const [message, , , snapshot, , eventType, incompleteReasons] = send.firstCall.args + assert.strictEqual(message, 'TypeError: boom') + assert.deepStrictEqual(snapshot.evaluationErrors, [{ expr: 'foo.bar == 42', message: 'TypeError: boom' }]) + assert.strictEqual(snapshot.captures, undefined) + assert.strictEqual(eventType, EVENT_TYPE.SNAPSHOT) + assert.strictEqual(incompleteReasons, 0) + }) + + it('should report a generic message if the recorded error is no longer available', async function () { + const probe = genProcessedProbe('probe-1') + probe.when = { dsl: 'foo.bar == 42', json: {} } + probe.condition = 'foo.bar === 42' + sampleProbe(probe, CONDITION_ERROR_FLAG) + + session.post = sinon.stub().callsFake((method) => { + if (method === 'Debugger.evaluateOnCallFrame') return Promise.resolve({ result: { value: [{}, undefined] } }) + return Promise.resolve({}) + }) + + await onPaused(event) + + sinon.assert.calledOnce(send) + const [message, , , snapshot, , eventType] = send.firstCall.args + assert.strictEqual(message, 'Unknown evaluation error') + assert.deepStrictEqual(snapshot.evaluationErrors, [ + { expr: 'foo.bar == 42', message: 'Unknown evaluation error' }, + ]) + assert.strictEqual(eventType, EVENT_TYPE.LOG) + }) + + it('should report condition errors alongside results of other probes at the same location', async function () { + const erroring = genProcessedProbe('probe-1') + erroring.when = { dsl: 'foo.bar == 42', json: {} } + erroring.condition = 'foo.bar === 42' + const templated = genProcessedProbe('probe-2') + templated.templateRequiresEvaluation = true + templated.template = '["hello ", foo]' + + state.breakpointToProbes.set(breakpointId, new Map([[erroring.id, erroring], [templated.id, templated]])) + state.samplingIndexToProbe.set(1, erroring) + state.samplingIndexToProbe.set(2, templated) + Atomics.store(sampledProbeIndexes, 0, 2) + Atomics.store(sampledProbeIndexes, 2, 2) + Atomics.store(sampledProbeIndexes, 3, 1 | CONDITION_ERROR_FLAG) + + session.post = sinon.stub().callsFake((method, params) => { + if (method === 'Debugger.evaluateOnCallFrame') { + // Frame expressions are evaluated in sampling order: the template first, then the condition error + const expectedSuffix = `,${templated.template},${takeConditionErrorExpression}]` + assert.strictEqual(params.expression.slice(-expectedSuffix.length), expectedSuffix) + return Promise.resolve({ result: { value: [{}, ['hello ', 'world'], 'TypeError: boom'] } }) + } + return Promise.resolve({}) + }) + + await onPaused(event) + + sinon.assert.calledTwice(send) + assert.strictEqual(send.firstCall.args[0], 'hello world') + assert.strictEqual(send.firstCall.args[3].evaluationErrors, undefined) + assert.strictEqual(send.secondCall.args[0], 'TypeError: boom') + assert.deepStrictEqual(send.secondCall.args[3].evaluationErrors, [ + { expr: 'foo.bar == 42', message: 'TypeError: boom' }, + ]) + }) + }) + it('should log sampler overflow', async function () { state.breakpointToProbes.set(breakpointId, new Map()) Atomics.store(sampledProbeIndexes, 1, 1) diff --git a/packages/dd-trace/test/debugger/probe_sampler.spec.js b/packages/dd-trace/test/debugger/probe_sampler.spec.js index e3f89ad4e56..0d0d2282e68 100644 --- a/packages/dd-trace/test/debugger/probe_sampler.spec.js +++ b/packages/dd-trace/test/debugger/probe_sampler.spec.js @@ -6,6 +6,8 @@ const { beforeEach, describe, it } = require('mocha') require('../setup/mocha') const { + CONDITION_ERROR_FLAG, + CONDITION_ERROR_THROTTLE_NS, MAX_SAMPLED_PROBES_PER_PAUSE, SAMPLED_PROBE_COUNT_INDEX, SAMPLED_PROBE_INDEXES_START, @@ -16,12 +18,22 @@ const { installProbeSampler, uninstallProbeSampler } = require('../../src/debugg const { compileBreakpointCondition, getRemoveProbeExpression, + getTakeConditionErrorExpression, } = require('../../src/debugger/devtools_client/probe_sampler') const { MAX_SNAPSHOTS_PER_SECOND_GLOBALLY } = require('../../src/debugger/devtools_client/defaults') const ddTraceSymbol = Symbol.for('dd-trace') const samplerSymbol = Symbol.for('dd-trace.debugger.probeSampler') +/** + * @typedef {object} RuntimeSampler + * @property {Function} makeSampleDecision + * @property {Function} shouldEvaluateCondition + * @property {Function} conditionError + * @property {Function} takeConditionError + * @property {Function} remove + */ + /** @type {GuardrailMetrics} */ let guardrailMetrics @@ -97,11 +109,16 @@ describe('probe sampler', function () { const $dd_sampler = globalThis[Symbol.for("dd-trace")]?.[Symbol.for("dd-trace.debugger.probeSampler")] if ($dd_sampler === undefined) return false let $dd_sampled = false - try { - if (((foo) === (42)) === true) { - $dd_sampled = $dd_sampler.makeSampleDecision(0, "probe-1", 200000n, true) || $dd_sampled + if ($dd_sampler.shouldEvaluateCondition("probe-1")) { + try { + if (((foo) === (42)) === true) { + $dd_sampled = $dd_sampler.makeSampleDecision(0, "probe-1", 200000n, true) || $dd_sampled + } + } catch ($dd_error) { + $dd_sampled = $dd_sampler.conditionError(0, "probe-1", $dd_error) || + $dd_sampled } - } catch {} + } return $dd_sampled })()`) }) @@ -110,6 +127,33 @@ describe('probe sampler', function () { assert.strictEqual(getRemoveProbeExpression('probe-1'), 'globalThis[Symbol.for("dd-trace")]?.[Symbol.for("dd-trace.debugger.probeSampler")]?.remove("probe-1")') }) + + it('should compile an expression that takes the recorded condition error of a probe', function () { + assert.strictEqual(getTakeConditionErrorExpression('probe-1'), + 'globalThis[Symbol.for("dd-trace")]?.[Symbol.for("dd-trace.debugger.probeSampler")]' + + '?.takeConditionError("probe-1")') + }) + + it('should pause for a condition error and skip the condition until the throttle window has passed', function () { + installSampler() + const sampler = getSampler() + const probes = [{ id: 'probe-1', samplingIndex: 0, nsBetweenSampling: 0n, condition: 'foo.bar' }] + const breakpointCondition = compileBreakpointCondition(probes) + const evaluate = () => { + // eslint-disable-next-line no-new-func + return new Function('foo', `return ${breakpointCondition}`)(undefined) + } + + assert.strictEqual(evaluate(), true, 'should pause to report the error') + assert.strictEqual( + sampler.takeConditionError('probe-1'), + "TypeError: Cannot read properties of undefined (reading 'bar')" + ) + assert.strictEqual(evaluate(), false, 'should skip the condition while throttled') + + now += CONDITION_ERROR_THROTTLE_NS + assert.strictEqual(evaluate(), true, 'should evaluate the condition again once the throttle window has passed') + }) }) describe('runtime sampler', function () { @@ -251,6 +295,94 @@ describe('probe sampler', function () { assert.deepStrictEqual(drainGuardrailMetrics(), []) }) + describe('condition errors', function () { + it('should evaluate conditions of probes without a recorded error', function () { + installSampler() + + assert.strictEqual(getSampler().shouldEvaluateCondition('probe-1'), true) + }) + + it('should request a pause flagged as a condition error', function () { + const sampledProbeIndexes = installSampler() + const sampler = getSampler() + + assert.strictEqual(sampler.conditionError(7, 'probe-1', new TypeError('boom')), true) + assert.strictEqual(Atomics.load(sampledProbeIndexes, SAMPLED_PROBE_COUNT_INDEX), 1) + assert.strictEqual(Atomics.load(sampledProbeIndexes, SAMPLED_PROBE_INDEXES_START), 7 | CONDITION_ERROR_FLAG) + }) + + it('should hand over the recorded error once', function () { + installSampler() + const sampler = getSampler() + + sampler.conditionError(7, 'probe-1', new TypeError('boom')) + + assert.strictEqual(sampler.takeConditionError('probe-1'), 'TypeError: boom') + assert.strictEqual(sampler.takeConditionError('probe-1'), undefined) + assert.strictEqual(sampler.takeConditionError('unknown-probe'), undefined) + }) + + it('should describe non-error values thrown by a condition', function () { + installSampler() + const sampler = getSampler() + + sampler.conditionError(7, 'probe-1', 'a string') + assert.strictEqual(sampler.takeConditionError('probe-1'), 'a string') + + sampler.conditionError(7, 'probe-1', { not: 'an error' }) + assert.strictEqual(sampler.takeConditionError('probe-1'), 'Unknown evaluation error') + }) + + it('should throttle condition evaluation for the throttle window after an error', function () { + installSampler() + const sampler = getSampler() + + sampler.conditionError(7, 'probe-1', new TypeError('boom')) + + assert.strictEqual(sampler.shouldEvaluateCondition('probe-1'), false) + assert.strictEqual(sampler.shouldEvaluateCondition('probe-2'), true, 'should not affect other probes') + now += CONDITION_ERROR_THROTTLE_NS - 1n + assert.strictEqual(sampler.shouldEvaluateCondition('probe-1'), false) + now += 1n + assert.strictEqual(sampler.shouldEvaluateCondition('probe-1'), true) + }) + + it('should not apply the per-probe or global rate limits to condition errors', function () { + const sampledProbeIndexes = installSampler() + const sampler = getSampler() + + for (let i = 0; i < MAX_SNAPSHOTS_PER_SECOND_GLOBALLY; i++) { + assert.strictEqual(sampler.makeSampleDecision(i, `snapshot-${i}`, 0n, true), true) + } + assert.strictEqual(sampler.makeSampleDecision(99, 'probe-1', 1_000_000_000n, true), false) + + assert.strictEqual(sampler.conditionError(99, 'probe-1', new Error('boom')), true) + assert.strictEqual( + Atomics.load(sampledProbeIndexes, SAMPLED_PROBE_COUNT_INDEX), + MAX_SNAPSHOTS_PER_SECOND_GLOBALLY + 1 + ) + }) + + it('should skip the condition error when the shared buffer is full', function () { + const sampledProbeIndexes = installSampler() + Atomics.store(sampledProbeIndexes, SAMPLED_PROBE_COUNT_INDEX, MAX_SAMPLED_PROBES_PER_PAUSE) + + assert.strictEqual(getSampler().conditionError(7, 'probe-1', new Error('boom')), false) + assert.strictEqual(Atomics.load(sampledProbeIndexes, SAMPLED_PROBE_OVERFLOW_INDEX), 1) + }) + + it('should forget the recorded error and throttle when a probe is removed', function () { + installSampler() + const sampler = getSampler() + + sampler.conditionError(7, 'probe-1', new Error('boom')) + sampler.remove('probe-1') + + assert.strictEqual(sampler.shouldEvaluateCondition('probe-1'), true) + assert.strictEqual(sampler.takeConditionError('probe-1'), undefined) + }) + }) + it('should set overflow and skip probes when the shared buffer is full', function () { const sampledProbeIndexes = installSampler() assert.strictEqual(Atomics.load(sampledProbeIndexes, SAMPLED_PROBE_OVERFLOW_INDEX), 0) @@ -300,5 +432,5 @@ function getDatadogGlobal () { * Get the installed runtime sampler. */ function getSampler () { - return /** @type {{ makeSampleDecision: Function, remove: Function }} */ (getDatadogGlobal()[samplerSymbol]) + return /** @type {RuntimeSampler} */ (getDatadogGlobal()[samplerSymbol]) }