Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEBUG-3439 DEBUG-3440 remove prefixes from probe paths when path matching #4346

Merged
merged 5 commits into from
Feb 10, 2025

Conversation

p-datadog
Copy link
Member

@p-datadog p-datadog commented Feb 5, 2025

What does this PR do?

Loosens the restrictions on path matching to also try to match prefixes of probe paths to the paths that exist at runtime.

For example, suppose at runtime we have:
/app/app/controllers/hello_controller.rb

Previously, either exact full path or suffixes of the path were accepted:

  • /app/app/controllers/hello_controller.rb
  • app/controllers/hello_controller.rb
  • hello_controller.rb
    Now, paths that have extra subdirectories in front will also be accepted:
  • local/hello_controller.rb
  • local/controllers/hello_controller.rb

Motivation:
For projects that are stored in subdirectories in their source code repositories (e.g. monorepos), source code integration is providing a full path that has extra prefixes from the application's standpoint (and also runtime standpoint). These prefixes previously made such paths not instrumentable. With this PR, the extra prefixes will be removed to try to find a matching file.

Change log entry
Yes: loosen path matching in dynamic instrumentation

Additional Notes:
This change can cause potentially incorrect files to match if the desired file is not yet loaded.

In order to properly implement matching accounting for these extra prefixes, much more work is needed. The ideal (in my present opinion) matching algorithm is described in a comment in this diff. This PR only implements a quick fix to get Ruby DI out to customers as soon as possible.

How to test the change?
Unit tests and integration test are added.

@p-datadog p-datadog requested a review from a team as a code owner February 5, 2025 13:49
@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Feb 5, 2025

Datadog Report

Branch report: di-strip-path
Commit report: f5eaac9
Test service: dd-trace-rb

✅ 0 Failed, 22102 Passed, 1476 Skipped, 5m 49.63s Total Time
⌛ 1 Performance Regression

⌛ Performance Regressions vs Default Branch (1)

  • Rails integration tests for an application with a basic route POST request with a non-event-triggering request behaves like a POST 200 span is expected to eq 0 - rspec 3.17s (+2.59s, +443%) - Details

@pr-commenter
Copy link

pr-commenter bot commented Feb 5, 2025

Benchmarks

Benchmark execution time: 2025-02-05 14:20:24

Comparing candidate commit f5eaac9 in PR branch di-strip-path with baseline commit d0322ed in branch master.

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

scenario:profiler - sample timeline=false

  • 🟩 throughput [+0.706op/s; +0.723op/s] or [+12.111%; +12.397%]

scenario:tracing - Propagation - Datadog

  • 🟥 throughput [-2986.099op/s; -2913.295op/s] or [-9.023%; -8.803%]

scenario:tracing - Tracing.log_correlation

  • 🟥 throughput [-7340.898op/s; -7010.841op/s] or [-6.232%; -5.952%]

@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 97.71%. Comparing base (04cce61) to head (f5eaac9).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
lib/datadog/di/probe.rb 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4346      +/-   ##
==========================================
- Coverage   97.72%   97.71%   -0.02%     
==========================================
  Files        1368     1368              
  Lines       82998    83036      +38     
  Branches     4220     4226       +6     
==========================================
+ Hits        81113    81139      +26     
- Misses       1885     1897      +12     

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

@p-datadog
Copy link
Member Author

I have now verified this PR manually against debugger-demo-ruby.

1 similar comment
@p-datadog
Copy link
Member Author

I have now verified this PR manually against debugger-demo-ruby.

@p-datadog p-datadog merged commit fba70fd into master Feb 10, 2025
492 checks passed
@p-datadog p-datadog deleted the di-strip-path branch February 10, 2025 14:46
@github-actions github-actions bot added this to the 2.11.0 milestone Feb 10, 2025
p-datadog pushed a commit that referenced this pull request Feb 10, 2025
* di-worker-middleware:
  rubocop
  standard
  current_component does not always exist?
  DEBUG-3457 hack probe notifier worker starting
  DEBUG-3439 DEBUG-3440 remove prefixes from probe paths when path matching (#4346)
  Add workaround for buggy gcc warnings in our Ruby 2.7 image
  [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/13238622524
  Update specs with new names for crashtracking tags
  [PROF-11306] Upgrade libdatadog dependency to 16.0.1
  [NO-TICKET] Update crashtracker with libdatadog 15 breaking changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants