From 1ae023d3b412dd25746aefaaa25b2af1335bcf4d Mon Sep 17 00:00:00 2001 From: Thomas Watson Date: Mon, 17 Feb 2025 11:49:46 +0100 Subject: [PATCH] [DI] Use column number from source maps (#5279) --- eslint.config.mjs | 3 +- .../debugger/source-map-support.spec.js | 47 ++++++++++++++----- .../source-map-support/index.js.map | 1 - .../target-app/source-map-support/minify.js | 11 +++++ .../source-map-support/minify.min.js | 2 + .../source-map-support/minify.min.js.map | 1 + .../scripts/build-minifiy.sh | 6 +++ .../scripts/build-typescript.sh | 3 ++ .../{index.js => typescript.js} | 2 +- .../source-map-support/typescript.js.map | 1 + .../{index.ts => typescript.ts} | 0 .../dynamic-instrumentation/worker/index.js | 9 ++-- .../debugger/devtools_client/breakpoints.js | 16 ++++--- .../debugger/devtools_client/source-maps.js | 4 +- .../devtools_client/source-maps.spec.js | 14 +++--- 15 files changed, 86 insertions(+), 34 deletions(-) delete mode 100644 integration-tests/debugger/target-app/source-map-support/index.js.map create mode 100644 integration-tests/debugger/target-app/source-map-support/minify.js create mode 100644 integration-tests/debugger/target-app/source-map-support/minify.min.js create mode 100644 integration-tests/debugger/target-app/source-map-support/minify.min.js.map create mode 100755 integration-tests/debugger/target-app/source-map-support/scripts/build-minifiy.sh create mode 100755 integration-tests/debugger/target-app/source-map-support/scripts/build-typescript.sh rename integration-tests/debugger/target-app/source-map-support/{index.js => typescript.js} (93%) create mode 100644 integration-tests/debugger/target-app/source-map-support/typescript.js.map rename integration-tests/debugger/target-app/source-map-support/{index.ts => typescript.ts} (100%) diff --git a/eslint.config.mjs b/eslint.config.mjs index 33a8ab6a773..9060d303218 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -41,7 +41,8 @@ export default [ '**/versions', // This is effectively a node_modules tree. '**/acmeair-nodejs', // We don't own this. '**/vendor', // Generally, we didn't author this code. - 'integration-tests/debugger/target-app/source-map-support/index.js', // Generated + 'integration-tests/debugger/target-app/source-map-support/minify.min.js', // Generated + 'integration-tests/debugger/target-app/source-map-support/typescript.js', // Generated 'integration-tests/esbuild/out.js', // Generated 'integration-tests/esbuild/aws-sdk-out.js', // Generated 'packages/dd-trace/src/appsec/blocked_templates.js', // TODO Why is this ignored? diff --git a/integration-tests/debugger/source-map-support.spec.js b/integration-tests/debugger/source-map-support.spec.js index 232d07a7a3e..f843d103bfe 100644 --- a/integration-tests/debugger/source-map-support.spec.js +++ b/integration-tests/debugger/source-map-support.spec.js @@ -5,23 +5,46 @@ const { setup } = require('./utils') describe('Dynamic Instrumentation', function () { describe('source map support', function () { - const t = setup({ - testApp: 'target-app/source-map-support/index.js', - testAppSource: 'target-app/source-map-support/index.ts' - }) + describe('Different file extention (TypeScript)', function () { + const t = setup({ + testApp: 'target-app/source-map-support/typescript.js', + testAppSource: 'target-app/source-map-support/typescript.ts' + }) - beforeEach(t.triggerBreakpoint) + beforeEach(t.triggerBreakpoint) - it('should support source maps', function (done) { - t.agent.on('debugger-input', ({ payload: [{ 'debugger.snapshot': { probe: { location } } }] }) => { - assert.deepEqual(location, { - file: 'target-app/source-map-support/index.ts', - lines: ['9'] + it('should support source maps', function (done) { + t.agent.on('debugger-input', ({ payload: [{ 'debugger.snapshot': { probe: { location } } }] }) => { + assert.deepEqual(location, { + file: 'target-app/source-map-support/typescript.ts', + lines: ['9'] + }) + done() }) - done() + + t.agent.addRemoteConfig(t.rcConfig) + }) + }) + + describe('Column information required (Minified)', function () { + const t = setup({ + testApp: 'target-app/source-map-support/minify.min.js', + testAppSource: 'target-app/source-map-support/minify.js' }) - t.agent.addRemoteConfig(t.rcConfig) + beforeEach(t.triggerBreakpoint) + + it('should support source maps', function (done) { + t.agent.on('debugger-input', ({ payload: [{ 'debugger.snapshot': { probe: { location } } }] }) => { + assert.deepEqual(location, { + file: 'target-app/source-map-support/minify.js', + lines: ['6'] + }) + done() + }) + + t.agent.addRemoteConfig(t.rcConfig) + }) }) }) }) diff --git a/integration-tests/debugger/target-app/source-map-support/index.js.map b/integration-tests/debugger/target-app/source-map-support/index.js.map deleted file mode 100644 index c246badc05b..00000000000 --- a/integration-tests/debugger/target-app/source-map-support/index.js.map +++ /dev/null @@ -1 +0,0 @@ -{"version":3,"file":"index.js","sourceRoot":"","sources":["index.ts"],"names":[],"mappings":";;AAAA,OAAO,CAAC,eAAe,CAAC,CAAA;AAExB,uCAAwC;AAExC,IAAM,MAAM,GAAG,IAAA,wBAAY,EAAC,UAAC,GAAG,EAAE,GAAG;IACnC,wFAAwF;IAGxF,GAAG,CAAC,GAAG,CAAC,aAAa,CAAC,CAAA,CAAC,gBAAgB;AACzC,CAAC,CAAC,CAAA;AAEF,MAAM,CAAC,MAAM,CAAC,OAAO,CAAC,GAAG,CAAC,QAAQ,EAAE;;IAClC,MAAA,OAAO,CAAC,IAAI,wDAAG,EAAE,IAAI,EAAE,OAAO,CAAC,GAAG,CAAC,QAAQ,EAAE,CAAC,CAAA;AAChD,CAAC,CAAC,CAAA"} \ No newline at end of file diff --git a/integration-tests/debugger/target-app/source-map-support/minify.js b/integration-tests/debugger/target-app/source-map-support/minify.js new file mode 100644 index 00000000000..2baf395873b --- /dev/null +++ b/integration-tests/debugger/target-app/source-map-support/minify.js @@ -0,0 +1,11 @@ +require('dd-trace/init') + +const { createServer } = require('node:http') + +const server = createServer((req, res) => { + res.end('hello world') // BREAKPOINT: / +}) + +server.listen(process.env.APP_PORT, () => { + process.send?.({ port: process.env.APP_PORT }) +}) diff --git a/integration-tests/debugger/target-app/source-map-support/minify.min.js b/integration-tests/debugger/target-app/source-map-support/minify.min.js new file mode 100644 index 00000000000..782c1ebce15 --- /dev/null +++ b/integration-tests/debugger/target-app/source-map-support/minify.min.js @@ -0,0 +1,2 @@ +require("dd-trace/init");const{createServer}=require("node:http");const server=createServer((req,res)=>{res.end("hello world")});server.listen(process.env.APP_PORT,()=>{process.send?.({port:process.env.APP_PORT})}); +//# sourceMappingURL=minify.min.js.map \ No newline at end of file diff --git a/integration-tests/debugger/target-app/source-map-support/minify.min.js.map b/integration-tests/debugger/target-app/source-map-support/minify.min.js.map new file mode 100644 index 00000000000..b3737180fb7 --- /dev/null +++ b/integration-tests/debugger/target-app/source-map-support/minify.min.js.map @@ -0,0 +1 @@ +{"version":3,"sources":["integration-tests/debugger/target-app/source-map-support/minify.js"],"names":["require","createServer","server","req","res","end","listen","process","env","APP_PORT","send","port"],"mappings":"AAAAA,QAAQ,eAAe,EAEvB,KAAM,CAAEC,YAAa,EAAID,QAAQ,WAAW,EAE5C,MAAME,OAASD,aAAa,CAACE,IAAKC,OAChCA,IAAIC,IAAI,aAAa,CACvB,CAAC,EAEDH,OAAOI,OAAOC,QAAQC,IAAIC,SAAU,KAClCF,QAAQG,OAAO,CAAEC,KAAMJ,QAAQC,IAAIC,QAAS,CAAC,CAC/C,CAAC"} \ No newline at end of file diff --git a/integration-tests/debugger/target-app/source-map-support/scripts/build-minifiy.sh b/integration-tests/debugger/target-app/source-map-support/scripts/build-minifiy.sh new file mode 100755 index 00000000000..c2da767802f --- /dev/null +++ b/integration-tests/debugger/target-app/source-map-support/scripts/build-minifiy.sh @@ -0,0 +1,6 @@ +#!/usr/bin/env sh + +npx uglify-js integration-tests/debugger/target-app/source-map-support/minify.js \ + -o integration-tests/debugger/target-app/source-map-support/minify.min.js \ + --v8 \ + --source-map url=minify.min.js.map diff --git a/integration-tests/debugger/target-app/source-map-support/scripts/build-typescript.sh b/integration-tests/debugger/target-app/source-map-support/scripts/build-typescript.sh new file mode 100755 index 00000000000..e2bf9a5ab30 --- /dev/null +++ b/integration-tests/debugger/target-app/source-map-support/scripts/build-typescript.sh @@ -0,0 +1,3 @@ +#!/usr/bin/env sh + +npx --package=typescript -- tsc --sourceMap integration-tests/debugger/target-app/source-map-support/typescript.ts diff --git a/integration-tests/debugger/target-app/source-map-support/index.js b/integration-tests/debugger/target-app/source-map-support/typescript.js similarity index 93% rename from integration-tests/debugger/target-app/source-map-support/index.js rename to integration-tests/debugger/target-app/source-map-support/typescript.js index d0eff097384..de7a4b5e972 100644 --- a/integration-tests/debugger/target-app/source-map-support/index.js +++ b/integration-tests/debugger/target-app/source-map-support/typescript.js @@ -10,4 +10,4 @@ server.listen(process.env.APP_PORT, function () { var _a; (_a = process.send) === null || _a === void 0 ? void 0 : _a.call(process, { port: process.env.APP_PORT }); }); -//# sourceMappingURL=index.js.map \ No newline at end of file +//# sourceMappingURL=typescript.js.map \ No newline at end of file diff --git a/integration-tests/debugger/target-app/source-map-support/typescript.js.map b/integration-tests/debugger/target-app/source-map-support/typescript.js.map new file mode 100644 index 00000000000..0f09d937224 --- /dev/null +++ b/integration-tests/debugger/target-app/source-map-support/typescript.js.map @@ -0,0 +1 @@ +{"version":3,"file":"typescript.js","sourceRoot":"","sources":["typescript.ts"],"names":[],"mappings":";;AAAA,OAAO,CAAC,eAAe,CAAC,CAAA;AAExB,uCAAwC;AAExC,IAAM,MAAM,GAAG,IAAA,wBAAY,EAAC,UAAC,GAAG,EAAE,GAAG;IACnC,wFAAwF;IAGxF,GAAG,CAAC,GAAG,CAAC,aAAa,CAAC,CAAA,CAAC,gBAAgB;AACzC,CAAC,CAAC,CAAA;AAEF,MAAM,CAAC,MAAM,CAAC,OAAO,CAAC,GAAG,CAAC,QAAQ,EAAE;;IAClC,MAAA,OAAO,CAAC,IAAI,wDAAG,EAAE,IAAI,EAAE,OAAO,CAAC,GAAG,CAAC,QAAQ,EAAE,CAAC,CAAA;AAChD,CAAC,CAAC,CAAA"} \ No newline at end of file diff --git a/integration-tests/debugger/target-app/source-map-support/index.ts b/integration-tests/debugger/target-app/source-map-support/typescript.ts similarity index 100% rename from integration-tests/debugger/target-app/source-map-support/index.ts rename to integration-tests/debugger/target-app/source-map-support/typescript.ts diff --git a/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js b/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js index 9701baf82cf..19b5df31f22 100644 --- a/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js +++ b/packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js @@ -12,7 +12,7 @@ const { randomUUID } = require('crypto') // TODO: move debugger/devtools_client/session to common place const session = require('../../../debugger/devtools_client/session') // TODO: move debugger/devtools_client/source-maps to common place -const { getSourceMappedLine } = require('../../../debugger/devtools_client/source-maps') +const { getGeneratedPosition } = require('../../../debugger/devtools_client/source-maps') // TODO: move debugger/devtools_client/snapshot to common place const { getLocalStateForCallFrame } = require('../../../debugger/devtools_client/snapshot') // TODO: move debugger/devtools_client/state to common place @@ -104,16 +104,18 @@ async function addBreakpoint (probe) { log.warn(`Adding breakpoint at ${url}:${line}`) let lineNumber = line + let columnNumber = 0 if (sourceMapURL) { try { - lineNumber = await getSourceMappedLine(url, source, line, sourceMapURL) + ({ line: lineNumber, column: columnNumber } = await getGeneratedPosition(url, source, line, sourceMapURL)) } catch (err) { log.error('Error processing script with source map', err) } if (lineNumber === null) { log.error('Could not find generated position for %s:%s', url, line) lineNumber = line + columnNumber = 0 } } @@ -121,7 +123,8 @@ async function addBreakpoint (probe) { const { breakpointId } = await session.post('Debugger.setBreakpoint', { location: { scriptId, - lineNumber: lineNumber - 1 + lineNumber: lineNumber - 1, + columnNumber } }) diff --git a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js index 5f4e764d6b8..229abf42df6 100644 --- a/packages/dd-trace/src/debugger/devtools_client/breakpoints.js +++ b/packages/dd-trace/src/debugger/devtools_client/breakpoints.js @@ -1,6 +1,6 @@ 'use strict' -const { getSourceMappedLine } = require('./source-maps') +const { getGeneratedPosition } = require('./source-maps') 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') @@ -17,10 +17,11 @@ async function addBreakpoint (probe) { if (!sessionStarted) await start() const file = probe.where.sourceFile - let line = Number(probe.where.lines[0]) // Tracer doesn't support multiple-line breakpoints + let lineNumber = Number(probe.where.lines[0]) // Tracer doesn't support multiple-line breakpoints + let columnNumber = 0 // Probes do not contain/support column information // Optimize for sending data to /debugger/v1/input endpoint - probe.location = { file, lines: [String(line)] } + probe.location = { file, lines: [String(lineNumber)] } delete probe.where // Optimize for fast calculations when probe is hit @@ -38,18 +39,19 @@ async function addBreakpoint (probe) { const { url, scriptId, sourceMapURL, source } = script if (sourceMapURL) { - line = await getSourceMappedLine(url, source, line, sourceMapURL) + ({ line: lineNumber, column: columnNumber } = await getGeneratedPosition(url, source, lineNumber, sourceMapURL)) } log.debug( - '[debugger:devtools_client] Adding breakpoint at %s:%d (probe: %s, version: %d)', - url, line, probe.id, probe.version + '[debugger:devtools_client] Adding breakpoint at %s:%d:%d (probe: %s, version: %d)', + url, lineNumber, columnNumber, probe.id, probe.version ) const { breakpointId } = await session.post('Debugger.setBreakpoint', { location: { scriptId, - lineNumber: line - 1 // Beware! lineNumber is zero-indexed + lineNumber: lineNumber - 1, // Beware! lineNumber is zero-indexed + columnNumber } }) diff --git a/packages/dd-trace/src/debugger/devtools_client/source-maps.js b/packages/dd-trace/src/debugger/devtools_client/source-maps.js index 79d89b62672..36e12f3e5bd 100644 --- a/packages/dd-trace/src/debugger/devtools_client/source-maps.js +++ b/packages/dd-trace/src/debugger/devtools_client/source-maps.js @@ -23,12 +23,12 @@ const self = module.exports = { return cacheIt(path, JSON.parse(readFileSync(path, 'utf8'))) }, - async getSourceMappedLine (url, source, line, sourceMapURL) { + async getGeneratedPosition (url, source, line, sourceMapURL) { const dir = dirname(new URL(url).pathname) return await SourceMapConsumer.with( await self.loadSourceMap(dir, sourceMapURL), null, - (consumer) => consumer.generatedPositionFor({ source, line, column: 0 }).line + (consumer) => consumer.generatedPositionFor({ source, line, column: 0 }) ) } } diff --git a/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js b/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js index d87a96f35d6..68cbea0986c 100644 --- a/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js +++ b/packages/dd-trace/test/debugger/devtools_client/source-maps.spec.js @@ -16,7 +16,7 @@ const rawSourceMap = JSON.stringify(parsedSourceMap) const inlineSourceMap = `data:application/json;base64,${Buffer.from(rawSourceMap).toString('base64')}` describe('source map utils', function () { - let loadSourceMap, loadSourceMapSync, getSourceMappedLine, readFileSync, readFile + let loadSourceMap, loadSourceMapSync, getGeneratedPosition, readFileSync, readFile describe('basic', function () { beforeEach(function () { @@ -30,7 +30,7 @@ describe('source map utils', function () { loadSourceMap = sourceMaps.loadSourceMap loadSourceMapSync = sourceMaps.loadSourceMapSync - getSourceMappedLine = sourceMaps.getSourceMappedLine + getGeneratedPosition = sourceMaps.getGeneratedPosition }) describe('loadSourceMap', function () { @@ -77,19 +77,19 @@ describe('source map utils', function () { }) }) - describe('getSourceMappedLine', function () { + describe('getGeneratedPosition', function () { const url = `file://${dir}/${parsedSourceMap.file}` const source = parsedSourceMap.sources[0] const line = 1 it('should return expected line for inline source map', async function () { - const result = await getSourceMappedLine(url, source, line, sourceMapURL) - expect(result).to.equal(2) + const pos = await getGeneratedPosition(url, source, line, sourceMapURL) + expect(pos).to.deep.equal({ line: 2, column: 0, lastColumn: 5 }) }) it('should return expected line for non-inline source map', async function () { - const result = await getSourceMappedLine(url, source, line, inlineSourceMap) - expect(result).to.equal(2) + const pos = await getGeneratedPosition(url, source, line, inlineSourceMap) + expect(pos).to.deep.equal({ line: 2, column: 0, lastColumn: 5 }) }) }) })