From 1fa77b39d841b8fe62db3d510501b338e04a200b Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Tue, 12 Nov 2024 15:42:28 +0100 Subject: [PATCH] [DI] Add support for sampling --- integration-tests/debugger/basic.spec.js | 39 +++++++++++++++++++ .../debugger/devtools_client/breakpoints.js | 8 ++++ .../src/debugger/devtools_client/defaults.js | 6 +++ .../src/debugger/devtools_client/index.js | 37 +++++++++++++++--- 4 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 packages/dd-trace/src/debugger/devtools_client/defaults.js diff --git a/integration-tests/debugger/basic.spec.js b/integration-tests/debugger/basic.spec.js index f42388396ef..22a8ec98ff1 100644 --- a/integration-tests/debugger/basic.spec.js +++ b/integration-tests/debugger/basic.spec.js @@ -338,6 +338,45 @@ describe('Dynamic Instrumentation', function () { }) }) + describe('sampling', function () { + it('should respect sampling rate for single probe', function (done) { + let start, timer + let payloadsReceived = 0 + const rcConfig = t.generateRemoteConfig({ sampling: { snapshotsPerSecond: 1 } }) + + function triggerBreakpointContinuously () { + t.axios.get(t.breakpoint.url).catch(done) + timer = setTimeout(triggerBreakpointContinuously, 10) + } + + t.agent.on('debugger-diagnostics', ({ payload }) => { + if (payload.debugger.diagnostics.status === 'INSTALLED') triggerBreakpointContinuously() + }) + + t.agent.on('debugger-input', () => { + payloadsReceived++ + if (payloadsReceived === 1) { + start = Date.now() + } else if (payloadsReceived === 2) { + const duration = Date.now() - start + clearTimeout(timer) + + // Allow for a variance of -5/+50ms (time will tell if this is enough) + assert.isAbove(duration, 995) + assert.isBelow(duration, 1050) + + // Wait at least a full sampling period, to see if we get any more payloads + timer = setTimeout(done, 1250) + } else { + clearTimeout(timer) + done(new Error('Too many payloads received!')) + } + }) + + t.agent.addRemoteConfig(rcConfig) + }) + }) + describe('race conditions', function () { it('should remove the last breakpoint completely before trying to add a new one', function (done) { const rcConfig2 = t.generateRemoteConfig() diff --git a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js index 5f12f83f11d..480c2479745 100644 --- a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js +++ b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js @@ -1,6 +1,7 @@ 'use strict' const session = require('./session') +const { MAX_SNAPSHOTS_PER_SECOND_PER_PROBE, MAX_NON_SNAPSHOTS_PER_SECOND_PER_PROBE } = require('./defaults') const { findScriptFromPartialPath, probes, breakpoints } = require('./state') const log = require('../../log') @@ -21,6 +22,13 @@ async function addBreakpoint (probe) { probe.location = { file, lines: [String(line)] } delete probe.where + // Optimize for fast calculations when probe is hit + const snapshotsPerSecond = probe.sampling.snapshotsPerSecond ?? (probe.captureSnapshot + ? MAX_SNAPSHOTS_PER_SECOND_PER_PROBE + : MAX_NON_SNAPSHOTS_PER_SECOND_PER_PROBE) + probe.sampling.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? diff --git a/packages/dd-trace/src/debugger/devtools_client/defaults.js b/packages/dd-trace/src/debugger/devtools_client/defaults.js new file mode 100644 index 00000000000..6acb813ab26 --- /dev/null +++ b/packages/dd-trace/src/debugger/devtools_client/defaults.js @@ -0,0 +1,6 @@ +'use strict' + +module.exports = { + MAX_SNAPSHOTS_PER_SECOND_PER_PROBE: 1, + MAX_NON_SNAPSHOTS_PER_SECOND_PER_PROBE: 5_000 +} diff --git a/packages/dd-trace/src/debugger/devtools_client/index.js b/packages/dd-trace/src/debugger/devtools_client/index.js index db71e7028e7..2397161d1b7 100644 --- a/packages/dd-trace/src/debugger/devtools_client/index.js +++ b/packages/dd-trace/src/debugger/devtools_client/index.js @@ -18,23 +18,47 @@ require('./remote_config') const threadId = parentThreadId === 0 ? `pid:${process.pid}` : `pid:${process.pid};tid:${parentThreadId}` const threadName = parentThreadId === 0 ? 'MainThread' : `WorkerThread:${parentThreadId}` +// WARNING: The code above the line `await session.post('Debugger.resume')` is highly optimized. Please edit with care! session.on('Debugger.paused', async ({ params }) => { const start = process.hrtime.bigint() - const timestamp = Date.now() let captureSnapshotForProbe = null let maxReferenceDepth, maxCollectionSize, maxFieldCount, maxLength - const probes = params.hitBreakpoints.map((id) => { + + // V8 doesn't allow seting more than one breakpoint at a specific location, however, it's possible to set two + // breakpoints just next to eachother that will "snap" to the same logical location, which in turn will be hit at the + // same time. E.g. index.js:1:1 and index.js:1:2. + // TODO: Investigate if it will improve performance to create a fast-path for when there's only a single breakpoint + let sampled = false + const length = params.hitBreakpoints.length + let probes = new Array(length) + for (let i = 0; i < length; i++) { + const id = params.hitBreakpoints[i] const probe = breakpoints.get(id) - if (probe.captureSnapshot) { + + if (start - probe.lastCaptureNs < probe.sampling.nsBetweenSampling) { + continue + } + + sampled = true + probe.lastCaptureNs = start + + if (probe.captureSnapshot === true) { captureSnapshotForProbe = probe maxReferenceDepth = highestOrUndefined(probe.capture.maxReferenceDepth, maxReferenceDepth) maxCollectionSize = highestOrUndefined(probe.capture.maxCollectionSize, maxCollectionSize) maxFieldCount = highestOrUndefined(probe.capture.maxFieldCount, maxFieldCount) maxLength = highestOrUndefined(probe.capture.maxLength, maxLength) } - return probe - }) + + probes[i] = probe + } + + if (sampled === false) { + return session.post('Debugger.resume') + } + + const timestamp = Date.now() let processLocalState if (captureSnapshotForProbe !== null) { @@ -56,6 +80,9 @@ session.on('Debugger.paused', async ({ params }) => { log.debug(`Finished processing breakpoints - main thread paused for: ${Number(diff) / 1000000} ms`) + // Due to the highly optimized algorithm above, the `probes` array might have gaps + probes = probes.filter((probe) => !!probe) + const logger = { // We can safely use `location.file` from the first probe in the array, since all probes hit by `hitBreakpoints` // must exist in the same file since the debugger can only pause the main thread in one location.