Skip to content

Commit 46c325b

Browse files
authored
fix(debugger): address TS errors (#6966)
Address a large majority of the TS type errors in the debugger code. The only actual change to the runtime logic is the following: When adding a new breakpoint to a file that has a source map, we now validate that the generated position can be identified. If not, we abort and don't add the breakpoint (which most likely would have failed anyway). An error will be sent to the debugger diagnostics endpoint if that happens. Another minor, but hopefully invisible change, is that all private fields in the `JSONBuffer` class are now properly private instead of just being prefixed with an underscore. The only debugger TS errors not addressed in this commit are the following: - The ones already addressed in #6965 - The ones which will disappear once #6951 lands This commit also adds a `tsconfig.debugger.json` file, which can be used to check the types of the debugger code: ./node_modules/.bin/tsc --noEmit -p tsconfig.debugger.json
1 parent fcddd26 commit 46c325b

17 files changed

Lines changed: 99 additions & 39 deletions

File tree

integration-tests/debugger/basic.spec.js

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,11 @@ describe('Dynamic Instrumentation', function () {
124124
const expected = expectedPayloads.shift()
125125
assertObjectContains(event, expected)
126126
assertUUID(event.debugger.diagnostics.runtimeId)
127-
if (event.debugger.diagnostics.status === 'INSTALLED') triggers.shift()()
127+
if (event.debugger.diagnostics.status === 'INSTALLED') {
128+
const trigger = triggers.shift()
129+
assert.ok(trigger, 'expecting a trigger function to be defined')
130+
trigger()
131+
}
128132
endIfDone()
129133
})
130134
})
@@ -195,12 +199,14 @@ describe('Dynamic Instrumentation', function () {
195199

196200
it(
197201
'should send expected error diagnostics messages if probe type isn\'t supported',
202+
// @ts-expect-error Expecting this probe type to be invalid
198203
unsupporedOrInvalidProbesTest(t.generateProbeConfig({ type: 'INVALID_PROBE' }))
199204
)
200205

201206
it(
202207
'should send expected error diagnostics messages if it isn\'t a line-probe',
203208
unsupporedOrInvalidProbesTest(
209+
// @ts-expect-error Expecting this probe type to be invalid
204210
t.generateProbeConfig({ where: { typeName: 'index.js', methodName: 'handlerA' } })
205211
)
206212
)
@@ -513,7 +519,11 @@ describe('Dynamic Instrumentation', function () {
513519

514520
t.agent.on('debugger-diagnostics', ({ payload }) => {
515521
payload.forEach((event) => {
516-
if (event.debugger.diagnostics.status === 'INSTALLED') triggers.shift()().catch(done)
522+
if (event.debugger.diagnostics.status === 'INSTALLED') {
523+
const trigger = triggers.shift()
524+
assert.ok(trigger, 'expecting a trigger function to be defined')
525+
trigger().catch(done)
526+
}
517527
})
518528
})
519529

@@ -587,6 +597,12 @@ describe('Dynamic Instrumentation', function () {
587597
})
588598

589599
it('should adhere to individual probes sample rate', function (done) {
600+
/** @type {(() => void) & { calledOnce?: boolean }} */
601+
const doneWhenCalledTwice = () => {
602+
if (doneWhenCalledTwice.calledOnce) return done()
603+
doneWhenCalledTwice.calledOnce = true
604+
}
605+
590606
const rcConfig1 = t.breakpoints[0].generateRemoteConfig({ sampling: { snapshotsPerSecond: 1 } })
591607
const rcConfig2 = t.breakpoints[1].generateRemoteConfig({ sampling: { snapshotsPerSecond: 1 } })
592608
const state = {
@@ -632,11 +648,6 @@ describe('Dynamic Instrumentation', function () {
632648

633649
t.agent.addRemoteConfig(rcConfig1)
634650
t.agent.addRemoteConfig(rcConfig2)
635-
636-
function doneWhenCalledTwice () {
637-
if (doneWhenCalledTwice.calledOnce) return done()
638-
doneWhenCalledTwice.calledOnce = true
639-
}
640651
})
641652
})
642653

integration-tests/debugger/target-app/basic.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict'
22

33
require('dd-trace/init')
4+
// @ts-expect-error This code is running in a sandbox where fastify is available
45
const Fastify = require('fastify')
56

67
const fastify = Fastify({ logger: { level: 'error' } })

integration-tests/debugger/target-app/redact.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict'
22

33
require('dd-trace/init')
4+
// @ts-expect-error This code is running in a sandbox where fastify is available
45
const Fastify = require('fastify')
56

67
const fastify = Fastify({ logger: { level: 'error' } })

integration-tests/debugger/target-app/snapshot-pruning.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
require('dd-trace/init')
44

55
const { randomBytes } = require('crypto')
6+
// @ts-expect-error This code is running in a sandbox where fastify is available
67
const Fastify = require('fastify')
78

89
const fastify = Fastify({ logger: { level: 'error' } })

integration-tests/debugger/target-app/snapshot-time-budget.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
require('dd-trace/init')
44

5+
// @ts-expect-error This code is running in a sandbox where fastify is available
56
const Fastify = require('fastify')
67

78
const fastify = Fastify({ logger: { level: 'error' } })

integration-tests/debugger/target-app/snapshot.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict'
22

33
require('dd-trace/init')
4+
// @ts-expect-error This code is running in a sandbox where fastify is available
45
const Fastify = require('fastify')
56

67
const fastify = Fastify({ logger: { level: 'error' } })

integration-tests/debugger/target-app/template.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
require('dd-trace/init')
44
const { inspect } = require('util')
5+
// @ts-expect-error This code is running in a sandbox where fastify is available
56
const Fastify = require('fastify')
67

78
const fastify = Fastify({ logger: { level: 'error' } })

integration-tests/debugger/utils.js

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ const pollInterval = 0.1
2222
* GenerateProbeConfigFn
2323
*/
2424

25+
/**
26+
* Bound version of generateProbeConfig that only requires optional overrides (breakpoint is already bound).
27+
*
28+
* @typedef {(overrides?: Partial<ProbeConfig>) => ProbeConfig} BoundGenerateProbeConfigFn
29+
*/
30+
2531
/**
2632
* @typedef {Object} BreakpointInfo
2733
* @property {string} sourceFile
@@ -37,7 +43,7 @@ const pollInterval = 0.1
3743
* rcConfig: object|null,
3844
* triggerBreakpoint: (url: string) => Promise<import('axios').AxiosResponse<unknown>>,
3945
* generateRemoteConfig: (overrides?: object) => object,
40-
* generateProbeConfig: GenerateProbeConfigFn
46+
* generateProbeConfig: BoundGenerateProbeConfigFn
4147
* }} EnrichedBreakpoint
4248
*/
4349

@@ -59,7 +65,7 @@ const pollInterval = 0.1
5965
* @property {() => Promise<import('axios').AxiosResponse<unknown>>} triggerBreakpoint - Triggers the primary breakpoint
6066
* once installed.
6167
* @property {(overrides?: object) => object} generateRemoteConfig - Generates RC for the primary breakpoint.
62-
* @property {GenerateProbeConfigFn} generateProbeConfig - Generates probe config for the primary breakpoint.
68+
* @property {BoundGenerateProbeConfigFn} generateProbeConfig - Generates probe config for the primary breakpoint.
6369
*/
6470

6571
module.exports = {

packages/dd-trace/src/debugger/devtools_client/breakpoints.js

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,17 @@ async function addBreakpoint (probe) {
8080
log.debug(
8181
'[debugger:devtools_client] Translating location using source map for %s:%d:%d (probe: %s, version: %d)',
8282
file, lineNumber, columnNumber, probe.id, probe.version
83-
);
84-
({ line: lineNumber, column: columnNumber } = await getGeneratedPosition(url, source, lineNumber, sourceMapURL))
83+
)
84+
const position = await getGeneratedPosition(url, source, lineNumber, sourceMapURL)
85+
if (position.line !== null && position.column !== null) {
86+
lineNumber = position.line
87+
columnNumber = position.column
88+
} else {
89+
throw new Error(
90+
// eslint-disable-next-line @stylistic/max-len
91+
`Could not find generated position for ${url}:${lineNumber}:${columnNumber} (probe: ${probe.id}, version: ${probe.version})`
92+
)
93+
}
8594
}
8695

8796
try {

packages/dd-trace/src/debugger/devtools_client/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ let snapshotProbeIndexBuffer, snapshotProbeIndex
4646

4747
if (SUPPORT_ARRAY_BUFFER_RESIZE) {
4848
// TODO: Is a limit of 256 snapshots ever going to be a problem?
49+
// @ts-ignore - ArrayBuffer constructor with maxByteLength is available in Node.js 20+ but not in @types/node@18
4950
// eslint-disable-next-line n/no-unsupported-features/es-syntax
5051
snapshotProbeIndexBuffer = new ArrayBuffer(1, { maxByteLength: 256 })
5152
// TODO: Is a limit of 256 probes ever going to be a problem?

0 commit comments

Comments
 (0)