[DI] Add support for probe condition by watson · Pull Request #5488 · DataDog/dd-trace-js

watson · 2025-03-26T14:55:07Z

What does this PR do?

Add support for having a condition on a probe.

A condition should return either true or false. It can however also throw an error. The error can be thrown in two places:

When compiling the AST down to the JavaScript source code (an issue was detected at compilation-time)
When running the condition in the context of the breakpoint (an issue was detected at run-time)

Compilation-time exceptions are reported as an error event on the probe to the DI diagnostics backend. Run-time exceptions are currently just swallowed and the result of the condition is going to be the same as if it evaluated false (i.e. it's not going to tigger the breakpoint).

To be 100% compatible with how conditions work in the other tracers, we should technically record the run-time errors and expose those to the user (as a help in debugging the condition). However, due to how conditions are implemented in this PR (by using the condition property on the Debugger.setBreakpoint Chrome DevTools Protocol API), it's not possible to know if a condition failed, or just returned false.

For now, this is an ok compromise, due to the increased performance gained by using this API.

Motivation

Feature parity with the other tracers
Improved performance for probes on hot code-paths that doesn't need to trigger all the time

Plugin Checklist

Additional Notes

watson · 2025-03-26T14:55:24Z

[DI] Add support for probe condition #5488 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-03-26T14:56:08Z

Overall package size

Self size: 9.19 MB
Deduped: 101.72 MB
No deduping: 102.23 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.0 | 29.83 MB | 29.83 MB | | @datadog/native-appsec | 8.5.1 | 19.26 MB | 19.27 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.6.0 | 9.79 MB | 10.16 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.4.0 | 2.77 MB | 5.42 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.13.1 | 117.64 kB | 839.26 kB | | source-map | 0.7.4 | 226 kB | 226 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.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | dc-polyfill | 0.1.6 | 24.56 kB | 24.56 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 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}

codecov · 2025-03-26T14:56:41Z

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.18%. Comparing base (b4b2969) to head (6a8f5f0).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/dd-trace/src/debugger/index.js	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5488      +/-   ##
==========================================
- Coverage   79.18%   79.18%   -0.01%     
==========================================
  Files         512      512              
  Lines       23156    23158       +2     
==========================================
+ Hits        18337    18338       +1     
- Misses       4819     4820       +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

datadog-datadog-prod-us1 · 2025-03-26T15:03:35Z

Datadog Report

Branch report: watson/DEBUG-2626/probe-condition
Commit report: 93248f1
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 919 Passed, 0 Skipped, 14m 6.04s Total Time

pr-commenter · 2025-03-26T15:05:38Z

Benchmarks

Benchmark execution time: 2025-04-01 11:53:56

Comparing candidate commit 6a8f5f0 in PR branch watson/DEBUG-2626/probe-condition with baseline commit b4b2969 in branch master.

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

shatzi

awesome progress

shatzi · 2025-03-26T15:25:43Z

should this be under Array.isArray(value) at line 39 ?

Is it because value[1] should also be compiled?

nope. because you access it as an array...

Under the Array.isArray(value) if-block below, I also compile the second element of the value array. If we don't want that in this case, it shouldn't be part of that if-block

shatzi · 2025-03-26T15:44:09Z

does the spec allow the user to have '@it' iterator over entities? not sure if that something we need to support..

I've been wondering the same thing. I came to the conclusion that shouldn't be supported based on this discussion from January, but maybe I misunderstood it.

open to interpretation. I would say: @it = [@key, @value] as entires return pairs.

another thing I don't like about the implementation is that @key get encoded as key. so if someone would like to write something: contains(keys, { @key == key }) it will fail. can we make @it, @key and @value unique so they can't override existing locals? or maybe add a long prefix like $__it or something crazy like that... I wish we can name variables using symbols but I don't think this is a thing....

Consensus from Zoom: We'll keep the current implementation

shatzi · 2025-03-27T04:36:44Z

all those errors should be more helpful for the user to understand the problem.
something like Possibility of side effect accessing when evaluate ${stringifyExpression(node)}

However, we might want to address this on future PRs

This only truly becomes relevant when implementing template strings, as any errors thrown in conditions will just treat the condition as "not met" - but the error itself will be swallowed.

I'll see if I can improve it in this PR, or I will defer it till I implement support for template strings.

Co-authored-by: Idan Shatz <shatzi@gmail.com>

BridgeAR · 2025-04-01T12:40:31Z

+      })
+
+      it('should report error if condition is invalid', function (done) {
+        t.agent.on('debugger-diagnostics', ({ payload }) => {


For future reference: using EventEmitter's once() would allow the tests to likely be nicer to read :)

Add support for having a condition on a probe. A condition should return either `true` or `false`. It can however also throw an error. The error can be thrown in two places: 1. When compiling the AST down to the JavaScript source code (an issue was detected at compilation-time) 2. When running the condition in the context of the breakpoint (an issue was detected at run-time) Compilation-time exceptions are reported as an error event on the probe to the DI diagnostics backend. Run-time exceptions are currently just swallowed and the result of the condition is going to be the same as if it evaluated `false` (i.e. it's not going to tigger the breakpoint). To be 100% compatible with how conditions work in the other tracers, we should technically record the run-time errors and expose those to the user (as a help in debugging the condition). However, due to how conditions are implemented in this PR (by using the `condition` property on the `Debugger.setBreakpoint` Chrome DevTools Protocol API), it's not possible to know if a condition failed, or just returned `false`. For now, this is an ok compromise, due to the increased performance gained by using this API.

watson self-assigned this Mar 26, 2025

watson added semver-minor debugger Dynamic Instrumentation & Live Debugger labels Mar 26, 2025

github-advanced-security AI found potential problems Mar 26, 2025

View reviewed changes

shatzi previously approved these changes Mar 26, 2025

View reviewed changes

watson dismissed shatzi’s stale review via 14cd322 March 26, 2025 20:34

github-advanced-security AI found potential problems Mar 26, 2025

View reviewed changes

watson force-pushed the watson/DEBUG-2626/probe-condition branch 2 times, most recently from d5189b6 to 948d0fb Compare March 26, 2025 21:17

shatzi reviewed Mar 27, 2025

View reviewed changes

Comment thread packages/dd-trace/src/debugger/devtools_client/condition.js Outdated

shatzi reviewed Mar 27, 2025

View reviewed changes

watson force-pushed the watson/DEBUG-2626/probe-condition branch from 948d0fb to 625ff93 Compare March 29, 2025 12:17

github-advanced-security AI found potential problems Mar 29, 2025

View reviewed changes

watson requested a review from shatzi March 30, 2025 10:09

watson marked this pull request as ready for review March 30, 2025 16:22

watson requested review from a team as code owners March 30, 2025 16:22

BridgeAR reviewed Mar 31, 2025

View reviewed changes

shatzi previously approved these changes Mar 31, 2025

View reviewed changes

Comment thread packages/dd-trace/src/debugger/devtools_client/condition.js Outdated

watson dismissed shatzi’s stale review via 50bb8a6 April 1, 2025 04:58

watson added 5 commits April 1, 2025 07:24

[DI] Add support for probe condition

3179b8c

Address review comments

b4bf01a

Clean up

1ca5e17

Guard against proxies

fe6abd2

Use unique variable names

bd9e7da

watson and others added 18 commits April 1, 2025 07:24

Guard against Symbol.toPrimitive side effects

dc7d30c

Add a ton of side effect checks

1ed0e08

Code cleanup

23ded87

Improve test coverage

fc0c1ca

Improve code comments

34aedd8

Code cleanup

2dc18e2

No code injection

9b4c063

Relax some rules

948dad6

Improve readability and add some more tests

a5989bd

Fix

c5439fe

Improve error message

243b823

Add test

bccc6ef

Update packages/dd-trace/src/debugger/devtools_client/condition.js

cd1fe97

Co-authored-by: Idan Shatz <shatzi@gmail.com>

Fix comment grammar

94c170b

Improve 'matches' performance

35223a1

Support checking instanceof across realms

e227be8

Improve performance of filter

ef5f4ca

Address review comments

69d6713

watson force-pushed the watson/DEBUG-2626/probe-condition branch from 50bb8a6 to 69d6713 Compare April 1, 2025 06:15

github-advanced-security AI found potential problems Apr 1, 2025

View reviewed changes

Comment thread packages/dd-trace/src/debugger/devtools_client/condition.js Dismissed

Merge branch 'master' into watson/DEBUG-2626/probe-condition

6a8f5f0

BridgeAR reviewed Apr 1, 2025

View reviewed changes

watson requested review from BridgeAR and shatzi April 1, 2025 13:06

shatzi approved these changes Apr 1, 2025

View reviewed changes

watson merged commit d8677ee into master Apr 1, 2025
427 checks passed

watson deleted the watson/DEBUG-2626/probe-condition branch April 1, 2025 15:29

wconti27 mentioned this pull request Apr 8, 2025

v5.46.0 proposal #5551

Merged

Conversation

watson commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

Uh oh!

watson commented Mar 26, 2025

Uh oh!

github-actions Bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overall package size

Uh oh!

codecov Bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

datadog-datadog-prod-us1 Bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

Uh oh!

pr-commenter Bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

shatzi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

watson commented Mar 26, 2025 •

edited

Loading

github-actions Bot commented Mar 26, 2025 •

edited

Loading

codecov Bot commented Mar 26, 2025 •

edited

Loading

datadog-datadog-prod-us1 Bot commented Mar 26, 2025 •

edited

Loading

pr-commenter Bot commented Mar 26, 2025 •

edited

Loading