[test visibility] Simple dynamic instrumentation - test visibility client #4826

juan-fernandez · 2024-10-25T11:34:08Z

What does this PR do?

This creates a TestVisDynamicInstrumentation class which includes a simplified version of the logic in packages/dd-trace/src/debugger/index.js:

A worker_thread is started in TestVisDynamicInstrumentation#start that'll wait for messages from the parent process.
The only available message right now is for adding a breakpoint at a given file and line.
If the breakpoint is hit, the worker will send the snapshot back to the parent process.

Motivation

Continuing the work for the integration between test visibility and dynamic instrumentation started in #4821

Plugin Checklist

Unit tests.

github-actions · 2024-10-25T11:35:00Z

Overall package size

Self size: 7.82 MB
Deduped: 64.68 MB
No deduping: 65.02 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

pr-commenter · 2024-10-25T11:42:39Z

Benchmarks

Benchmark execution time: 2024-10-30 10:39:29

Comparing candidate commit bcd2b3e in PR branch juan-fernandez/add-di-simple-client with baseline commit 49d6c58 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 8 unstable metrics.

juan-fernandez · 2024-10-28T13:45:50Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

+const breakpointIdToProbe = new Map()
+
+function findScriptFromPartialPath (path) {
+  return scriptIds


ℹ️ this we could easily generalized for both DI and TV

juan-fernandez · 2024-10-28T13:47:23Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

+  const stack = callFrames.map((frame) => {
+    let fileName = scriptUrls.get(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
+    }
+  })


ℹ️ this can be extracted

juan-fernandez · 2024-10-28T13:51:11Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

@@ -0,0 +1,106 @@
+const { parentPort } = require('worker_threads')
+// TODO: move session to common place
+const session = require('../../../debugger/devtools_client/session')


session should be simple to move

on second thought, session should probably go together with inspector_promises_polyfill.js and state.js since they seem to be self contained? I'm honestly OK with importing code directly here, as this code is evolving quickly. I don't know if it makes sense to invest on this now

thoughts @watson ?

TL;DR: Leave session.js where it is for now.

This file currently acts a singleton, so it can't both be called from your code and from DI in the same worker thread. But I don't assume that's ever going to happen - just an observation.

If we were to move this to a generic shared location, there's a chance that others might use it for other things in the future. If so, this singleton behavior might throw someone for a loop. But since it's such simple code, I don't see any good reason to make this into a shared module. The inspector_promises_polyfill.js file on the other hand, should me moved to a central location.

So normally my conclusion to the above would be that we should each have our own version of session.js that just called a shared inspector_promises_polyfill.js. However, since the current session.js so used all over the place in my code, it would mean that all that would have to be refactored to allow a session object to be passed into the called functions. And I think that's too big of a change right now - hence my comment at the top about leaving it as-is for now.

juan-fernandez · 2024-10-28T13:52:13Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

+// TODO: move session to common place
+const session = require('../../../debugger/devtools_client/session')
+// TODO: move getLocalStateForCallFrame to common place
+const { getLocalStateForCallFrame } = require('../../../debugger/devtools_client/snapshot')


this might be a bit trickier but no reason not to do it

Especially if we use two different session.js files as I just laid out in my previous comment. If so, we need a way to pass in the current session object. So for now, I'd just leave this as-is

juan-fernandez · 2024-10-29T11:29:02Z

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

@@ -32,8 +32,17 @@ module.exports = {
      .sort(([a], [b]) => a.length - b.length)[0]
  },

-  getScriptUrlFromId (id) {


getScriptUrlFromId was only used for formatting the stack, which is now done by getStackFromCallFrames

codecov · 2024-10-30T10:14:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.87%. Comparing base (564795f) to head (9d9f7dd).
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4826      +/-   ##
==========================================
- Coverage   79.17%   72.87%   -6.30%     
==========================================
  Files         273       66     -207     
  Lines       12427     3278    -9149     
==========================================
- Hits         9839     2389    -7450     
+ Misses       2588      889    -1699

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

juan-fernandez · 2024-10-30T10:14:38Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js

+  // 2. Promise that's resolved when the breakpoint is set
+  // 3. Promise that's resolved when the breakpoint is hit


I need to be able to sync on these two to be able to know when to start a test (a test can't start before the breakpoint is set) and when a dynamic instrumentation log can be created (if a breakpoint is hit). Maybe I could use callbacks for the same? Open to ideas here

juan-fernandez · 2024-10-30T10:15:15Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js

+  }
+
+  isReady () {
+    return this._readyPromise


this is just to make sure we don't attempt to set probes before the worker is online

juan-fernandez · 2024-10-30T10:15:54Z

...ibility/dynamic-instrumentation/target-app/test-visibility-dynamic-instrumentation-script.js

@@ -0,0 +1,29 @@
+'use strict'


ℹ️ script we run in the test to grab local variable data

watson · 2024-10-31T12:55:02Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js

+    this.worker.on('message', (message) => {
+      const { type } = message
+      if (type === BREAKPOINT_SET) {
+        const { probeId } = message
+        const resolve = probeIdToResolveBreakpointSet.get(probeId)
+        if (resolve) {
+          resolve()
+          probeIdToResolveBreakpointSet.delete(probeId)
+        }
+      } else if (type === BREAKPOINT_HIT) {
+        const { snapshot } = message
+        const { probe: { id: probeId } } = snapshot
+        const resolve = probeIdToResolveBreakpointHit.get(probeId)
+        if (resolve) {
+          resolve({ snapshot })
+          probeIdToResolveBreakpointHit.delete(probeId)
+        }
+      }
+    }).unref() // We also need to unref this message handler


Instead of using a type of BREAKPOINT_SET and BREAKPOINT_HIT that you send over the message channel, you could also just use two different distinct MessageChannel instances. That way you always knew that if a message was received on the breakpointSetChannel vs the breakpointHitChannel what it meant. And you'd also avoid this if/else blocks.

You can see how I use those here:

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

Lines 24 to 25 in 57f8a10

const rcChannel = new MessageChannel()

configChannel = new MessageChannel()

watson · 2024-10-31T13:00:18Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/index.js

+    this.worker.on('message', (message) => {
+      const { type } = message
+      if (type === BREAKPOINT_SET) {
+        const { probeId } = message
+        const resolve = probeIdToResolveBreakpointSet.get(probeId)
+        if (resolve) {
+          resolve()
+          probeIdToResolveBreakpointSet.delete(probeId)
+        }
+      } else if (type === BREAKPOINT_HIT) {
+        const { snapshot } = message
+        const { probe: { id: probeId } } = snapshot
+        const resolve = probeIdToResolveBreakpointHit.get(probeId)
+        if (resolve) {
+          resolve({ snapshot })
+          probeIdToResolveBreakpointHit.delete(probeId)
+        }
+      }
+    }).unref() // We also need to unref this message handler


I assume no calling .unref() here gave you problems?

watson · 2024-10-31T13:03:16Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

@@ -0,0 +1,88 @@
+const { parentPort } = require('worker_threads')


No 'use strict'?

watson · 2024-10-31T13:08:44Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

@@ -0,0 +1,106 @@
+const { parentPort } = require('worker_threads')
+// TODO: move session to common place
+const session = require('../../../debugger/devtools_client/session')


TL;DR: Leave session.js where it is for now.

This file currently acts a singleton, so it can't both be called from your code and from DI in the same worker thread. But I don't assume that's ever going to happen - just an observation.

If we were to move this to a generic shared location, there's a chance that others might use it for other things in the future. If so, this singleton behavior might throw someone for a loop. But since it's such simple code, I don't see any good reason to make this into a shared module. The inspector_promises_polyfill.js file on the other hand, should me moved to a central location.

So normally my conclusion to the above would be that we should each have our own version of session.js that just called a shared inspector_promises_polyfill.js. However, since the current session.js so used all over the place in my code, it would mean that all that would have to be refactored to allow a session object to be passed into the called functions. And I think that's too big of a change right now - hence my comment at the top about leaving it as-is for now.

watson · 2024-10-31T13:12:35Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

+// TODO: move session to common place
+const session = require('../../../debugger/devtools_client/session')
+// TODO: move getLocalStateForCallFrame to common place
+const { getLocalStateForCallFrame } = require('../../../debugger/devtools_client/snapshot')


Especially if we use two different session.js files as I just laid out in my previous comment. If so, we need a way to pass in the current session object. So for now, I'd just leave this as-is

watson · 2024-10-31T13:23:12Z

packages/dd-trace/test/ci-visibility/dynamic-instrumentation/dynamic-instrumentation.spec.js

+    // Dynamic Instrumentation - Test Visibility not currently supported for windows
+    if (process.platform === 'win32') {
+      done()
+      return
+    }


Why not move this whole block to the root of the spec file before the first call to describe?

watson · 2024-10-31T13:26:00Z

packages/dd-trace/test/ci-visibility/dynamic-instrumentation/dynamic-instrumentation.spec.js

+
+      assert.deepEqual(captures, {
+        lines: {
+          9: {


Consider if you can re-use the getBreakpointInfo function for you tests. That way, if you ever change the line your breakpoint is on, your tests just adapt automatically.

watson · 2024-10-31T13:28:56Z

packages/dd-trace/src/ci-visibility/dynamic-instrumentation/worker/index.js

+  if (!probe) {
+    return session.post('Debugger.resume')
+  }


Is this just an escape hatch in case the state is somehow messed up, is there a valid use-case for a breakpoint to exist, but no matching probe?

If this is just an escape hatch, I'd probably add a log line to log that you encountered this issue. But I'll leave that up you :)

juan-fernandez added ci-app semver-minor labels Oct 25, 2024

juan-fernandez commented Oct 28, 2024

View reviewed changes

juan-fernandez added 4 commits October 29, 2024 11:40

add simple di-tv client

d017e4a

simplify

97cf8d3

simplify

4b221e5

even simpler

c20ae05

juan-fernandez force-pushed the juan-fernandez/add-di-simple-client branch from e794cc6 to c20ae05 Compare October 29, 2024 10:40

better org

cacd571

juan-fernandez commented Oct 29, 2024

View reviewed changes

add e2e test

bff4609

juan-fernandez changed the title ~~[wip] [test visibility] Simple dynamic instrumentation - test visibility client~~ [test visibility] Simple dynamic instrumentation - test visibility client Oct 29, 2024

better test org

9d9f7dd

juan-fernandez commented Oct 30, 2024

View reviewed changes

do not run on windows

bcd2b3e

juan-fernandez marked this pull request as ready for review October 30, 2024 10:32

juan-fernandez requested review from a team as code owners October 30, 2024 10:32

juan-fernandez requested review from vitor-de-araujo and watson October 30, 2024 10:32

vitor-de-araujo approved these changes Oct 30, 2024

View reviewed changes

watson reviewed Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test visibility] Simple dynamic instrumentation - test visibility client #4826

[test visibility] Simple dynamic instrumentation - test visibility client #4826

juan-fernandez commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

pr-commenter bot commented Oct 25, 2024 •

edited

Loading

juan-fernandez Oct 28, 2024

juan-fernandez Oct 28, 2024

juan-fernandez Oct 28, 2024

juan-fernandez Oct 30, 2024

juan-fernandez Oct 30, 2024

watson Oct 31, 2024

juan-fernandez Oct 28, 2024

watson Oct 31, 2024

juan-fernandez Oct 29, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

juan-fernandez Oct 30, 2024

juan-fernandez Oct 30, 2024

juan-fernandez Oct 30, 2024

watson Oct 31, 2024

watson Oct 31, 2024

watson Oct 31, 2024

watson Oct 31, 2024

watson Oct 31, 2024

watson Oct 31, 2024

watson Oct 31, 2024

watson Oct 31, 2024

		// 2. Promise that's resolved when the breakpoint is set
		// 3. Promise that's resolved when the breakpoint is hit

	const rcChannel = new MessageChannel()
	configChannel = new MessageChannel()

		@@ -0,0 +1,88 @@
		const { parentPort } = require('worker_threads')

[test visibility] Simple dynamic instrumentation - test visibility client #4826

Are you sure you want to change the base?

[test visibility] Simple dynamic instrumentation - test visibility client #4826

Conversation

juan-fernandez commented Oct 25, 2024 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

github-actions bot commented Oct 25, 2024 • edited Loading

Overall package size

pr-commenter bot commented Oct 25, 2024 • edited Loading

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juan-fernandez commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024 •

edited

Loading

pr-commenter bot commented Oct 25, 2024 •

edited

Loading

codecov bot commented Oct 30, 2024 •

edited

Loading