Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions integration-tests/debugger/conditions.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,39 @@ describe('Dynamic Instrumentation', function () {
}))
})

it('should report an error result if the condition throws, once per throttle window', function (done) {
const rcConfig = t.generateRemoteConfig({
captureSnapshot: true,
when: { dsl: 'definitelyDoesNotExist == "never"', json: { eq: [{ ref: 'definitelyDoesNotExist' }, 'never'] } },
})
let results = 0

t.agent.on('debugger-input', ({ payload }) => {
results += payload.length
assert.strictEqual(payload.length, 1)
const { message, debugger: { snapshot } } = payload[0]

assert.strictEqual(message, 'ReferenceError: definitelyDoesNotExist is not defined')
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)

// Every further hit within the throttle window is skipped without evaluating the condition again
Promise.all([t.request(t.breakpoint.url), t.request(t.breakpoint.url)])
.then(() => new Promise((resolve) => setTimeout(resolve, 1500)))
.then(() => {
assert.strictEqual(results, 1, 'should only report the condition error once')
done()
})
.catch(done)
})

t.agent.addRemoteConfig(rcConfig)
})

it('should report error if condition cannot be compiled', function (done) {
const rcConfig = t.generateRemoteConfig({
when: { dsl: 'original dsl', json: { ref: 'this is not a valid ref' } },
Expand Down
15 changes: 12 additions & 3 deletions integration-tests/debugger/diagnostics.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,12 @@ describe('Dynamic Instrumentation', function () {

it('should support not triggering any probes when all conditions are not met', function (done) {
let installed = 0
const rcConfig1 = t.generateRemoteConfig({ when: { json: { eq: [{ ref: 'foo' }, 'bar'] } } })
const rcConfig2 = t.generateRemoteConfig({ when: { json: { eq: [{ ref: 'foo' }, 'baz'] } } })
const rcConfig1 = t.generateRemoteConfig({
when: { json: { eq: [{ getmember: [{ getmember: [{ ref: 'request' }, 'params'] }, 'name'] }, 'invalid'] } },
})
const rcConfig2 = t.generateRemoteConfig({
when: { json: { eq: [{ getmember: [{ getmember: [{ ref: 'request' }, 'params'] }, 'name'] }, 'nope'] } },
})

t.agent.on('debugger-diagnostics', ({ payload }) => {
payload.forEach((event) => {
Expand Down Expand Up @@ -394,12 +398,17 @@ describe('Dynamic Instrumentation', function () {

it('trigger on met condition, even if other condition throws (all have conditions)', function (done) {
let installed = 0
// this condition will throw because `foo` is not defined
// this condition will throw because `foo` is not defined, which is reported as an error result
const rcConfig1 = t.generateRemoteConfig({ when: { json: { eq: [{ ref: 'foo' }, 'bar'] } } })
const rcConfig2 = t.generateRemoteConfig({
when: { json: { eq: [{ getmember: [{ getmember: [{ ref: 'request' }, 'params'] }, 'name'] }, 'bar'] } },
})
const expectedPayloads = new Map([
[rcConfig1.config.id, {
ddsource: 'dd_debugger',
service: 'node',
debugger: { diagnostics: { probeId: rcConfig1.config.id, probeVersion: 0, status: 'EMITTING' } },
}],
[rcConfig2.config.id, {
ddsource: 'dd_debugger',
service: 'node',
Expand Down
44 changes: 38 additions & 6 deletions packages/dd-trace/src/debugger/devtools_client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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<object> | 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
Expand All @@ -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) {
Expand All @@ -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++
Expand All @@ -109,7 +125,7 @@ session.on('Debugger.paused', async ({ params }) => {
}

if (probe.templateRequiresEvaluation) {
templateExpressions += `,${probe.template}`
frameExpressions += `,${probe.template}`
}

probes.push(probe)
Expand All @@ -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,
})
Expand Down Expand Up @@ -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
Expand All @@ -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<typeof localState>} */ (localState)
Expand Down
26 changes: 22 additions & 4 deletions packages/dd-trace/src/debugger/devtools_client/probe_sampler.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const SAMPLER_EXPRESSION = `globalThis[Symbol.for(${JSON.stringify(DD_TRACE_SYMB
module.exports = {
compileBreakpointCondition,
getRemoveProbeExpression,
getTakeConditionErrorExpression,
}

/**
Expand All @@ -21,6 +22,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.
Expand Down Expand Up @@ -72,9 +84,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 {}`
}`
}
89 changes: 83 additions & 6 deletions packages/dd-trace/src/debugger/probe_sampler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<string, { throttledUntilNs: bigint, error: string | undefined }>}
*/
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)
Expand Down Expand Up @@ -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) {
Expand All @@ -85,20 +90,80 @@ 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.
*
* @param {string} probeId - The probe id.
*/
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
}

Expand All @@ -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.
*
Expand Down
9 changes: 9 additions & 0 deletions packages/dd-trace/src/debugger/probe_sampler_constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading
Loading