[DI] Improve error handling during snapshot collection (#5719)

This changes what happens if an error occurs while gathering the local
state for the snapshot.

Before this change, an `ERROR` event would be emitted to the diagnostics
endpoint, while still sending a probe result without any snapshot to the
intake, followed by an `EMITTING` event. However, not being able to
collect the snapshot does not warrant emitting an `ERROR` event. This
commit changes that behaviour to instead report the snapshot with a
`notCapturedReason` without emitting an `ERROR` event.

Author: watson

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

@@ -6,7 +6,7 @@ const session = require('./session')
 const { getLocalStateForCallFrame } = require('./snapshot')
 const send = require('./send')
 const { getStackFromCallFrames } = require('./state')
-const { ackEmitting, ackError } = require('./status')
+const { ackEmitting } = require('./status')
 const { parentThreadId } = require('./config')
 const { MAX_SNAPSHOTS_PER_SECOND_GLOBALLY } = require('./defaults')
 const log = require('../../log')
@@ -144,20 +144,11 @@ session.on('Debugger.paused', async ({ params }) => {
     evalResults = result?.value ?? []
   }
 
-  let processLocalState
-  if (numberOfProbesWithSnapshots !== 0) {
-    try {
-      // TODO: Create unique states for each affected probe based on that probes unique `capture` settings (DEBUG-2863)
-      processLocalState = await getLocalStateForCallFrame(
-        params.callFrames[0],
-        { maxReferenceDepth, maxCollectionSize, maxFieldCount, maxLength }
-      )
-    } catch (err) {
-      for (let i = 0; i < numberOfProbesWithSnapshots; i++) {
-        ackError(err, probes[snapshotProbeIndex[i]]) // TODO: Ok to continue after sending ackError?
-      }
-    }
-  }
+  // TODO: Create unique states for each affected probe based on that probes unique `capture` settings (DEBUG-2863)
+  const processLocalState = numberOfProbesWithSnapshots !== 0 && await getLocalStateForCallFrame(
+    params.callFrames[0],
+    { maxReferenceDepth, maxCollectionSize, maxFieldCount, maxLength }
+  )
 
   await session.post('Debugger.resume')
   const diff = process.hrtime.bigint() - start // TODO: Recored as telemetry (DEBUG-2858)
@@ -197,7 +188,9 @@ session.on('Debugger.paused', async ({ params }) => {
 
     if (probe.captureSnapshot) {
       const state = processLocalState()
-      if (state) {
+      if (state instanceof Error) {
+        snapshot.captureError = state.message
+      } else if (state) {
         snapshot.captures = {
           lines: { [probe.location.lines[0]]: { locals: state } }
         }

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

@@ -2,6 +2,7 @@
 
 const { getRuntimeObject } = require('./collector')
 const { processRawState } = require('./processor')
+const log = require('../../../log')
 
 const DEFAULT_MAX_REFERENCE_DEPTH = 3
 const DEFAULT_MAX_COLLECTION_SIZE = 100
@@ -24,13 +25,20 @@ async function getLocalStateForCallFrame (
   const rawState = []
   let processedState = null
 
-  await Promise.all(callFrame.scopeChain.map(async (scope) => {
-    if (scope.type === 'global') return // The global scope is too noisy
-    rawState.push(...await getRuntimeObject(
-      scope.object.objectId,
-      { maxReferenceDepth, maxCollectionSize, maxFieldCount }
-    ))
-  }))
+  try {
+    await Promise.all(callFrame.scopeChain.map(async (scope) => {
+      if (scope.type === 'global') return // The global scope is too noisy
+      rawState.push(...await getRuntimeObject(
+        scope.object.objectId,
+        { maxReferenceDepth, maxCollectionSize, maxFieldCount }
+      ))
+    }))
+  } catch (err) {
+    // TODO: We might be able to get part of the scope chain.
+    // Consider if we could set errors just for the part of the scope chain that throws during collection.
+    log.error('[debugger:devtools_client] Error getting local state for call frame', err)
+    return () => new Error('Error getting local state')
+  }
 
   // Deplay calling `processRawState` so the caller gets a chance to resume the main thread before processing `rawState`
   return () => {

packages/dd-trace/test/debugger/devtools_client/snapshot/error-handling.spec.js

@@ -0,0 +1,20 @@
+'use strict'
+
+require('../../../setup/mocha')
+
+const assert = require('node:assert')
+
+require('./stub-session')
+const { getLocalStateForCallFrame } = require('../../../../src/debugger/devtools_client/snapshot')
+
+describe('debugger -> devtools client -> snapshot.getLocalStateForCallFrame', function () {
+  describe('error handling', function () {
+    it('should generate a notCapturedReason if an error is thrown during inital collection', async function () {
+      const invalidCallFrameThatTriggersAnException = {}
+      const processLocalState = await getLocalStateForCallFrame(invalidCallFrameThatTriggersAnException)
+      const result = processLocalState()
+      assert.ok(result instanceof Error)
+      assert.strictEqual(result.message, 'Error getting local state')
+    })
+  })
+})

packages/dd-trace/test/debugger/devtools_client/snapshot/stub-session.js

@@ -0,0 +1,19 @@
+'use strict'
+
+const inspector = require('../../../../src/debugger/devtools_client/inspector_promises_polyfill')
+const session = module.exports = new inspector.Session()
+session.connect()
+
+session['@noCallThru'] = true
+proxyquire('../src/debugger/devtools_client/snapshot/collector', {
+  '../session': session
+})
+proxyquire('../src/debugger/devtools_client/snapshot/redaction', {
+  '../config': {
+    dynamicInstrumentation: {
+      redactedIdentifiers: [],
+      redactionExcludedIdentifiers: []
+    },
+    '@noCallThru': true
+  }
+})

packages/dd-trace/test/debugger/devtools_client/snapshot/utils.js

@@ -2,24 +2,7 @@
 
 const { join, basename } = require('path')
 
-const inspector = require('../../../../src/debugger/devtools_client/inspector_promises_polyfill')
-const session = new inspector.Session()
-session.connect()
-
-session['@noCallThru'] = true
-proxyquire('../src/debugger/devtools_client/snapshot/collector', {
-  '../session': session
-})
-proxyquire('../src/debugger/devtools_client/snapshot/redaction', {
-  '../config': {
-    dynamicInstrumentation: {
-      redactedIdentifiers: [],
-      redactionExcludedIdentifiers: []
-    },
-    '@noCallThru': true
-  }
-})
-
+const session = require('./stub-session')
 const { getLocalStateForCallFrame } = require('../../../../src/debugger/devtools_client/snapshot')
 
 module.exports = {