From 37af6066ad31651c0a3d7a719942d90c72c60752 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Thu, 26 Sep 2024 13:14:11 +0200 Subject: [PATCH 1/2] [DI] Add stack trace to log probe results This stack trace represents the point in the execution when the breakpoint associated with the probe was hit. The strack frames also include a `columnNumber` property, even though the API doesn't yet use this property. However, support for column information in the backend is being added in parallel. --- integration-tests/debugger/index.spec.js | 16 ++++++++++++++++ .../src/debugger/devtools_client/index.js | 13 +++++++++++++ .../debugger/devtools_client/remote_config.js | 4 ++-- .../src/debugger/devtools_client/state.js | 14 ++++++++++---- 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/integration-tests/debugger/index.spec.js b/integration-tests/debugger/index.spec.js index 241f57f722f..4f975e179f6 100644 --- a/integration-tests/debugger/index.spec.js +++ b/integration-tests/debugger/index.spec.js @@ -308,6 +308,22 @@ describe('Dynamic Instrumentation', function () { assert.isTrue(payload['debugger.snapshot'].timestamp > Date.now() - 1000 * 60) assert.isTrue(payload['debugger.snapshot'].timestamp <= Date.now()) + assert.isArray(payload['debugger.snapshot'].stack) + assert.isAbove(payload['debugger.snapshot'].stack.length, 0) + for (const frame of payload['debugger.snapshot'].stack) { + assert.isObject(frame) + assert.hasAllKeys(frame, ['fileName', 'function', 'lineNumber', 'columnNumber']) + assert.isString(frame.fileName) + assert.isString(frame.function) + assert.isAbove(frame.lineNumber, 0) + assert.isAbove(frame.columnNumber, 0) + } + const topFrame = payload['debugger.snapshot'].stack[0] + assert.match(topFrame.fileName, new RegExp(`${appFile}$`)) // path seems to be prefeixed with `/private` on Mac + assert.strictEqual(topFrame.function, 'handler') + assert.strictEqual(topFrame.lineNumber, probeLineNo) + assert.strictEqual(topFrame.columnNumber, 3) + done() }) diff --git a/packages/dd-trace/src/debugger/devtools_client/index.js b/packages/dd-trace/src/debugger/devtools_client/index.js index f4789ea65a8..e1100f99ab7 100644 --- a/packages/dd-trace/src/debugger/devtools_client/index.js +++ b/packages/dd-trace/src/debugger/devtools_client/index.js @@ -4,6 +4,7 @@ const { randomUUID } = require('crypto') const { breakpoints } = require('./state') const session = require('./session') const send = require('./send') +const { getScriptUrlFromId } = require('./state') const { ackEmitting } = require('./status') const { parentThreadId } = require('./config') const log = require('../../log') @@ -35,6 +36,17 @@ session.on('Debugger.paused', async ({ params }) => { thread_name: threadName } + const stack = params.callFrames.map((frame) => { + let fileName = getScriptUrlFromId(frame.location.scriptId) + if (fileName.startsWith('file://')) fileName = fileName.substr(7) // TODO: This might not be required + return { + fileName, + function: frame.functionName, + lineNumber: frame.location.lineNumber + 1, // Beware! lineNumber is zero-indexed + columnNumber: frame.location.columnNumber + 1 // Beware! columnNumber is zero-indexed + } + }) + // TODO: Send multiple probes in one HTTP request as an array for (const probe of probes) { const snapshot = { @@ -45,6 +57,7 @@ session.on('Debugger.paused', async ({ params }) => { version: probe.version, location: probe.location }, + stack, language: 'javascript' } diff --git a/packages/dd-trace/src/debugger/devtools_client/remote_config.js b/packages/dd-trace/src/debugger/devtools_client/remote_config.js index 25ac070cc9f..b030b093728 100644 --- a/packages/dd-trace/src/debugger/devtools_client/remote_config.js +++ b/packages/dd-trace/src/debugger/devtools_client/remote_config.js @@ -1,7 +1,7 @@ 'use strict' const { workerData: { rcPort } } = require('node:worker_threads') -const { getScript, probes, breakpoints } = require('./state') +const { findScriptFromPartialPath, probes, breakpoints } = require('./state') const session = require('./session') const { ackReceived, ackInstalled, ackError } = require('./status') const log = require('../../log') @@ -120,7 +120,7 @@ async function addBreakpoint (probe) { // 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? - const script = getScript(file) + const script = findScriptFromPartialPath(file) if (!script) throw new Error(`No loaded script found for ${file} (probe: ${probe.id}, version: ${probe.version})`) const [path, scriptId] = script diff --git a/packages/dd-trace/src/debugger/devtools_client/state.js b/packages/dd-trace/src/debugger/devtools_client/state.js index 316841667fb..8be9c808369 100644 --- a/packages/dd-trace/src/debugger/devtools_client/state.js +++ b/packages/dd-trace/src/debugger/devtools_client/state.js @@ -2,7 +2,8 @@ const session = require('./session') -const scripts = [] +const scriptIds = [] +const scriptUrls = new Map() module.exports = { probes: new Map(), @@ -25,10 +26,14 @@ module.exports = { * @param {string} path * @returns {[string, string] | undefined} */ - getScript (path) { - return scripts + findScriptFromPartialPath (path) { + return scriptIds .filter(([url]) => url.endsWith(path)) .sort(([a], [b]) => a.length - b.length)[0] + }, + + getScriptUrlFromId (id) { + return scriptUrls.get(id) } } @@ -41,7 +46,8 @@ module.exports = { // - `` - Not sure what this is, but should just be ignored // TODO: Event fired for all files, every time debugger is enabled. So when we disable it, we need to reset the state session.on('Debugger.scriptParsed', ({ params }) => { + scriptUrls.set(params.scriptId, params.url) if (params.url.startsWith('file:')) { - scripts.push([params.url, params.scriptId]) + scriptIds.push([params.url, params.scriptId]) } }) From 1c2a37c7043199f90acc03d7ed96ae06044be229 Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Thu, 26 Sep 2024 13:36:29 +0200 Subject: [PATCH 2/2] [DI] Improve performance of algorithm to find probe scriptId --- .../src/debugger/devtools_client/state.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/dd-trace/src/debugger/devtools_client/state.js b/packages/dd-trace/src/debugger/devtools_client/state.js index 8be9c808369..0eab8654885 100644 --- a/packages/dd-trace/src/debugger/devtools_client/state.js +++ b/packages/dd-trace/src/debugger/devtools_client/state.js @@ -2,7 +2,7 @@ const session = require('./session') -const scriptIds = [] +const scriptIds = new Map() const scriptUrls = new Map() module.exports = { @@ -24,12 +24,18 @@ module.exports = { * To fix this, specify a more unique file path, e.g `demo-project1/index.js` instead of `index.js` * * @param {string} path - * @returns {[string, string] | undefined} + * @returns {[string, string] | null} */ findScriptFromPartialPath (path) { - return scriptIds - .filter(([url]) => url.endsWith(path)) - .sort(([a], [b]) => a.length - b.length)[0] + let matchUrl = null + let matchId = null + for (const [url, id] of scriptIds) { + if (url.endsWith(path) && (matchUrl === null || url.length < matchUrl.length)) { + matchUrl = url + matchId = id + } + } + return matchUrl === null ? null : [matchUrl, matchId] }, getScriptUrlFromId (id) { @@ -48,6 +54,6 @@ module.exports = { session.on('Debugger.scriptParsed', ({ params }) => { scriptUrls.set(params.scriptId, params.url) if (params.url.startsWith('file:')) { - scriptIds.push([params.url, params.scriptId]) + scriptIds.set(params.url, params.scriptId) } })