diff --git a/integration-tests/debugger/conditions.spec.js b/integration-tests/debugger/conditions.spec.js index 0e7fbc5f93..c42bf18740 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 9e29e29118..ba407012a8 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 f2dcc30241..428dd801a3 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 c30a882204..3eee7ea67d 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 b9b153da33..6eec3acdc1 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 2b67bcdddf..11cfce8172 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 97e46b5c2f..700566ef45 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 e3f89ad4e5..0d0d2282e6 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]) }