[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.91 MB
Deduped: 64.93 MB
No deduping: 65.27 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 | 3.0.1 | 1.06 MB | 1.46 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-11-04 14:20:26

Comparing candidate commit 67b7647 in PR branch juan-fernandez/add-di-simple-client with baseline commit 83fcef6 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

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

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

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

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?

yeah, the test framework I used to run my tests got stuck and I had to pkill node

Odd... I never had this problem 🤔 @tlhunter @bengl Does this ring a bell?

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

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

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

…ient (#4826)

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

juan-fernandez commented Oct 28, 2024

View reviewed changes

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

juan-fernandez commented Oct 28, 2024

View reviewed changes

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

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 previously approved these changes Oct 30, 2024

View reviewed changes

watson reviewed Oct 31, 2024

View reviewed changes

use message channels instead of types

9dcc8df

juan-fernandez dismissed vitor-de-araujo’s stale review via 9dcc8df November 4, 2024 13:58

juan-fernandez added 4 commits November 4, 2024 15:03

use strict

c92c401

add log warning if no probe is found

cd4b6fe

bail out of windows earlier in test

ec347a2

Merge branch 'master' into juan-fernandez/add-di-simple-client

67b7647

juan-fernandez requested review from watson and vitor-de-araujo November 4, 2024 14:20

vitor-de-araujo approved these changes Nov 4, 2024

View reviewed changes

juan-fernandez merged commit 0b4dab7 into master Nov 6, 2024
206 checks passed

juan-fernandez deleted the juan-fernandez/add-di-simple-client branch November 6, 2024 08:29

rochdev pushed a commit that referenced this pull request Nov 6, 2024

[test visibility] Simple dynamic instrumentation - test visibility cl…

875f46a

…ient (#4826)

rochdev pushed a commit that referenced this pull request Nov 6, 2024

[test visibility] Simple dynamic instrumentation - test visibility cl…

8a4221d

…ient (#4826)

rochdev pushed a commit that referenced this pull request Nov 6, 2024

[test visibility] Simple dynamic instrumentation - test visibility cl…

f35449e

…ient (#4826)

rochdev pushed a commit that referenced this pull request Nov 6, 2024

[test visibility] Simple dynamic instrumentation - test visibility cl…

564f254

…ient (#4826)

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 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

juan-fernandez Nov 4, 2024

watson Nov 7, 2024

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

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

[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

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

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