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-3182 DI: remove hard dependency on tracing #4223

Merged
merged 3 commits into from
Dec 17, 2024
Merged

Conversation

p-datadog
Copy link
Member

What does this PR do?

Removes hard dependency of DI on Tracing.

  • Existing requires of tracing components from DI removed
  • DI will check if Datadog::Tracing exists before referencing current_span & current_trace
  • Unit tests adjusted to require their dependencies explicitly and to use local CodeTracker instances rather than the global one

Motivation:

DI code tracker must be loaded early in application boot process. Tracing must be loaded late to instrument third-party libraries. Currently, DI requires some tracing components which may result in tracing not loading correctly.

Change log entry

None - I am currently not aware of specific issues with DI or tracing loading.

Additional Notes:

I intend to add a test in a separate PR that loads DI code tracking in a forked process, verifies that the code tracking works correctly, and asserts that none of the other components (most importantly tracing but we could check others too) have been loaded.

How to test the change?

I tested the encapsulation manually by removing a bunch of requires pertaining to tracing from spec_helper.rb and other helpers. The main test of the DI code tracker both working correctly and not loading tracing will be in a later PR.

@p-datadog p-datadog requested a review from a team as a code owner December 12, 2024 18:19
@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Dec 12, 2024

Datadog Report

Branch report: di-undepend-tracing
Commit report: f0accc9
Test service: dd-trace-rb

✅ 0 Failed, 22103 Passed, 1457 Skipped, 5m 28.75s Total Time

@pr-commenter
Copy link

pr-commenter bot commented Dec 12, 2024

Benchmarks

Benchmark execution time: 2024-12-12 18:46:44

Comparing candidate commit f0accc9 in PR branch di-undepend-tracing with baseline commit 65fb81d in branch master.

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

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.75%. Comparing base (65fb81d) to head (f0accc9).

Files with missing lines Patch % Lines
lib/datadog/di/probe_notification_builder.rb 71.42% 2 Missing ⚠️
spec/datadog/di/spec_helper.rb 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4223      +/-   ##
==========================================
- Coverage   97.75%   97.75%   -0.01%     
==========================================
  Files        1355     1355              
  Lines       82165    82189      +24     
  Branches     4207     4213       +6     
==========================================
+ Hits        80321    80344      +23     
- Misses       1844     1845       +1     

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

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ivoanjo's point here, but overall awesome work! :shipit:

@p-datadog p-datadog merged commit 7349680 into master Dec 17, 2024
339 checks passed
@p-datadog p-datadog deleted the di-undepend-tracing branch December 17, 2024 13:42
@github-actions github-actions bot added this to the 2.9.0 milestone Dec 17, 2024
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.

6 participants