DEBUG-2334 dynamic instrumentation instrumenter component #4012

p-datadog · 2024-10-17T18:48:05Z

Change log entry

None, DI is not yet customer-visible.

What does this PR do?

This component instruments target code, meaning when target method or target line is invoked, the specified callback will be called with information about the program state at the target point (e.g. method parameters for method probe, local variables for line probe).

Motivation:
Initial implementation of Ruby dynamic instrumentation.

Additional Notes:
The instrumenter itself is under 300 lines. Most of the diff is in the test suite.

How to test the change?
This PR includes unit tests, but a large part of instrumentation is dealing with code that is loaded after the instrumentation is installed (since much Ruby code is lazily loaded) and test coverage for this will be added later since it requires more DI components.

Unsure? Have a question? Request a review!

This component instruments target code, meaning when target method or target line is invoked, the specified callback will be called with information about the program state at the target point (e.g. method parameters for method probe, local variables for line probe).

spec/datadog/di/instrumenter_spec.rb

spec/datadog/di/hook_line_recursive.rb

spec/datadog/di/instrumenter_spec.rb

lib/datadog/di/instrumenter.rb

lib/datadog/di/utils.rb

spec/datadog/di/instrumenter_spec.rb

spec/datadog/di/hook_method.rb

spec/datadog/di/instrumenter_spec.rb

Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>

spec/datadog/di/instrumenter_spec.rb

…t used

pr-commenter · 2024-10-17T19:23:06Z

Benchmarks

Benchmark execution time: 2024-10-17 20:01:02

Comparing candidate commit f904292 in PR branch di-instrumenter with baseline commit 60febf8 in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

🟥 throughput [-0.637op/s; -0.619op/s] or [-9.385%; -9.119%]

p-datadog · 2024-10-17T21:48:16Z

This PR has some xit tests, I am planning on reviewing them to either make them work or delete them from the tree.

codecov-commenter · 2024-10-17T21:50:50Z

Codecov Report

Attention: Patch coverage is 92.33716% with 40 lines in your changes missing coverage. Please review.

Project coverage is 97.79%. Comparing base (e8ef953) to head (8ef0a12).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/datadog/di/instrumenter.rb	83.49%	17 Missing ⚠️
spec/datadog/di/instrumenter_spec.rb	96.29%	12 Missing ⚠️
lib/datadog/di/probe.rb	57.14%	3 Missing ⚠️
lib/datadog/di.rb	87.50%	2 Missing ⚠️
spec/datadog/di/hook_line.rb	80.00%	1 Missing ⚠️
spec/datadog/di/hook_line_basic.rb	80.00%	1 Missing ⚠️
spec/datadog/di/hook_line_load.rb	80.00%	1 Missing ⚠️
spec/datadog/di/hook_line_recursive.rb	88.88%	1 Missing ⚠️
spec/datadog/di/hook_line_targeted.rb	80.00%	1 Missing ⚠️
spec/datadog/di/hook_method.rb	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4012      +/-   ##
==========================================
- Coverage   97.82%   97.79%   -0.04%     
==========================================
  Files        1327     1337      +10     
  Lines       79698    80212     +514     
  Branches     3952     4013      +61     
==========================================
+ Hits        77968    78441     +473     
- Misses       1730     1771      +41

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

spec/datadog/di/instrumenter_spec.rb

benchmarks/di_instrument.rb

spec/datadog/di/instrumenter_spec.rb

Strech

I have a few minor notes regarding code checker variable, but despite that 🟢

lib/datadog/di.rb

lib/datadog/di/utils.rb

sig/datadog/di.rbs

lib/datadog/di.rb

ivoanjo · 2024-10-29T07:24:51Z

lib/datadog/di/instrumenter.rb

+              duration = Benchmark.realtime do # steep:ignore
+                rv = super(*args, **kwargs)
+              end


I'm not sure this is delegation is correct -- there may be corner cases with args/keyword args; see https://eregon.me/blog/2019/11/10/the-delegation-challenge-of-ruby27.html for details. I think this is a quite sensitive part since getting it wrong means installing a probe can break the app.

(Also @lloeki quite likes this topic so I think he's the person to ping for a quick check on this bit ;D )

This is one of my items to address before initial release, but the "shortcut" is I can just mandate Ruby 3.0+ for DI if this problem is difficult to solve.

lib/datadog/di/utils.rb

spec/datadog/di/instrumenter_spec.rb

Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>

The DI code being benchmarked does not exist in master, making the baseline benchmark run fail. I will extract it into a separate PR.

These contain code for line probes and method probes, only method probes are currently enabled becuase more DI code is needed to have line probes work.

These contain code for line probes and method probes, only method probes are currently enabled becuase more DI code is needed to have line probes work. Co-authored-by: Oleg Pudeyev <code@olegp.name>

* master: (25 commits) lint update method fix unix path regex add back mixed_http_and_uds fix test update tests to match this new world order use uds Fix Intel case Purge circleci cache Udpate ffi to 1.17 for gemfiles/*lock Pin version across rubies Fix labeler Pin bundler version Improve naming for persistent_data arg for appsec waf [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/11611017851 Update libddwaf gem to v1.15.0.0.0 Add musl keys Reduce requirement to 2.27 Implement requirement.json DEBUG-2334 DI benchmarks extracted from DataDog#4012 (DataDog#4049) ...

p-datadog requested a review from a team as a code owner October 17, 2024 18:48

datadog-datadog-prod-us1 bot reviewed Oct 17, 2024

View reviewed changes

p-datadog and others added 3 commits October 17, 2024 14:51

Update spec/datadog/di/instrumenter_spec.rb

b62e696

Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>

Update spec/datadog/di/instrumenter_spec.rb

3af08d9

Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>

Update lib/datadog/di/instrumenter.rb

075e186

Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>

datadog-datadog-prod-us1 bot reviewed Oct 17, 2024

View reviewed changes

spec/datadog/di/instrumenter_spec.rb Outdated Show resolved Hide resolved

datadog-datadog-prod-us1 bot reviewed Oct 17, 2024

View reviewed changes

spec/datadog/di/instrumenter_spec.rb Show resolved Hide resolved

datadog-datadog-prod-us1 bot reviewed Oct 17, 2024

View reviewed changes

spec/datadog/di/instrumenter_spec.rb Show resolved Hide resolved

p added 4 commits October 17, 2024 14:59

instrumenter is now receiving code tracker explicitly, globals are no…

5e95dbe

…t used

add code tracker bits to DI module

d772a3e

need to clear code tracker

8b3a0c4

sig/steep

f904292

p added 5 commits October 17, 2024 15:23

annotate as di tests

04cf54e

run di benchmark in CI

23e19c0

fix tests in CI, hopefully

ff6d416

standard

f13bac4

more standard

329c66e

p-datadog added the dev/internal Other internal work that does not need to be included in the changelog label Oct 18, 2024

p and others added 6 commits October 18, 2024 12:37

fix di benchmark

6984bd9

Merge branch 'master' into di-instrumenter

b014993

change to caller_locations

691ef93

types

99a5f01

re-enable xit tests

bffcc83

dependency-inject logger

61329f8

datadog-datadog-prod-us1 bot reviewed Oct 25, 2024

View reviewed changes

spec/datadog/di/instrumenter_spec.rb Show resolved Hide resolved

benchmarks/di_instrument.rb Outdated Show resolved Hide resolved

datadog-datadog-prod-us1 bot reviewed Oct 25, 2024

View reviewed changes

spec/datadog/di/instrumenter_spec.rb Show resolved Hide resolved

p and others added 2 commits October 25, 2024 13:29

types

3cbf563

Merge branch 'master' into di-instrumenter

9451d35

p-datadog mentioned this pull request Oct 25, 2024

DEBUG-2334 Probe Notifier Worker component #4028

Merged

p added 3 commits October 25, 2024 14:39

exception boundaries at trace points

8648852

standard

1b2aa98

add missing require

83baea3

Strech approved these changes Oct 28, 2024

View reviewed changes

lib/datadog/di.rb Show resolved Hide resolved

lib/datadog/di/utils.rb Show resolved Hide resolved

sig/datadog/di.rbs Show resolved Hide resolved

ivoanjo reviewed Oct 29, 2024

View reviewed changes

y9v reviewed Oct 29, 2024

View reviewed changes

lib/datadog/di/utils.rb Show resolved Hide resolved

y9v reviewed Oct 29, 2024

View reviewed changes

spec/datadog/di/instrumenter_spec.rb Show resolved Hide resolved

p-datadog and others added 2 commits October 29, 2024 09:05

Update lib/datadog/di.rb

a20af20

Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>

more di requires

49478bc

p-datadog force-pushed the di-instrumenter branch from 385e486 to 49478bc Compare October 29, 2024 14:21

Merge branch 'master' into di-instrumenter

c6df3c6

y9v approved these changes Oct 29, 2024

View reviewed changes

p-datadog force-pushed the di-instrumenter branch 3 times, most recently from 2b6eed6 to 9cb5206 Compare October 29, 2024 15:35

remove benchmarks as they are failing against master

8ef0a12

The DI code being benchmarked does not exist in master, making the baseline benchmark run fail. I will extract it into a separate PR.

p-datadog force-pushed the di-instrumenter branch from 9cb5206 to 8ef0a12 Compare October 30, 2024 06:55

p-datadog merged commit fe0c0f2 into master Oct 30, 2024
269 of 270 checks passed

p-datadog deleted the di-instrumenter branch October 30, 2024 13:04

github-actions bot added this to the 2.5.0 milestone Oct 30, 2024

p-datadog pushed a commit that referenced this pull request Oct 30, 2024

DI benchmarks extracted from #4012

5a6a098

These contain code for line probes and method probes, only method probes are currently enabled becuase more DI code is needed to have line probes work.

p-datadog mentioned this pull request Oct 30, 2024

DEBUG-2334 DI benchmarks extracted from #4012 #4049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEBUG-2334 dynamic instrumentation instrumenter component #4012

DEBUG-2334 dynamic instrumentation instrumenter component #4012

p-datadog commented Oct 17, 2024 •

edited

Loading

pr-commenter bot commented Oct 17, 2024 •

edited

Loading

p-datadog commented Oct 17, 2024

codecov-commenter commented Oct 17, 2024 •

edited

Loading

Strech left a comment

ivoanjo Oct 29, 2024

p-datadog Oct 29, 2024

DEBUG-2334 dynamic instrumentation instrumenter component #4012

DEBUG-2334 dynamic instrumentation instrumenter component #4012

Conversation

p-datadog commented Oct 17, 2024 • edited Loading

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

Benchmarks

scenario:profiler - sample timeline=false

p-datadog commented Oct 17, 2024

codecov-commenter commented Oct 17, 2024 • edited Loading

Codecov Report

Strech left a comment

Choose a reason for hiding this comment

ivoanjo Oct 29, 2024

Choose a reason for hiding this comment

p-datadog Oct 29, 2024

Choose a reason for hiding this comment

p-datadog commented Oct 17, 2024 •

edited

Loading

pr-commenter bot commented Oct 17, 2024 •

edited

Loading

codecov-commenter commented Oct 17, 2024 •

edited

Loading