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-3251 dependency inject logger into DI component #4262

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

p-datadog
Copy link
Member

What does this PR do?

Alters DI component initialization to pass the logger into DI as a parameter rather than DI reading it out of Datadog module.

Motivation:

Logging should be passed into DI in one way.
Additionally, #4230 makes changes to how the logging is handled within DI, this PR makes the test suite log to mocks in several places and these places would need to be adjusted in addition to the other code in #4230.

Change log entry
None

Additional Notes:

How to test the change?

@p-datadog p-datadog requested a review from a team as a code owner January 7, 2025 16:07
@github-actions github-actions bot added the core Involves Datadog core libraries label Jan 7, 2025
@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Jan 7, 2025

Datadog Report

Branch report: di-inject-logger
Commit report: 6fb789e
Test service: dd-trace-rb

✅ 0 Failed, 22114 Passed, 1476 Skipped, 5m 33.05s Total Time

@p-datadog p-datadog added this to the 2.9.0 milestone Jan 7, 2025
@pr-commenter
Copy link

pr-commenter bot commented Jan 7, 2025

Benchmarks

Benchmark execution time: 2025-01-08 05:33:22

Comparing candidate commit 6fb789e in PR branch di-inject-logger with baseline commit 9440c03 in branch master.

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

scenario:profiler - Allocations ()

  • 🟥 throughput [-243693.461op/s; -227035.552op/s] or [-7.405%; -6.899%]

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.73%. Comparing base (9440c03) to head (6fb789e).

Files with missing lines Patch % Lines
lib/datadog/di/component.rb 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4262      +/-   ##
==========================================
- Coverage   97.73%   97.73%   -0.01%     
==========================================
  Files        1352     1352              
  Lines       82409    82419      +10     
  Branches     4224     4224              
==========================================
+ Hits        80545    80551       +6     
- Misses       1864     1868       +4     

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

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 LGTM

lib/datadog/di/component.rb Show resolved Hide resolved
@p-datadog p-datadog merged commit 3137dfb into master Jan 8, 2025
337 checks passed
@p-datadog p-datadog deleted the di-inject-logger branch January 8, 2025 14:02
p-datadog pushed a commit to p-datadog/dd-trace-rb that referenced this pull request Jan 9, 2025
* master:
  DEBUG-3210 DI: change logging to be appropriate for customer inspection (DataDog#4266)
  Report timing information if try_wait_until times out (DataDog#4265)
  Move simplecov patch to an overlay in our tree instead of using a forked simplecov repo (DataDog#4263)
  DEBUG-3251 dependency inject logger into DI component (DataDog#4262)
  DEBUG-3182 move Rails utils to core (DataDog#4261)
  add supported versions workflow (DataDog#4210)
  DEBUG-3305 remove dependency on benchmark (DataDog#4259)
  Fix case & grammar in issue template (DataDog#4244)
  [🤖] Update Latest Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/12614773826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants