Skip to content

Conversation

@vpellan
Copy link
Contributor

@vpellan vpellan commented May 9, 2025

What does this PR do?

This PR adds a default logger during configuration initialization.

Motivation:

During config initialization, stable config will be able to log errors related to reading or parsing config files. But when doing this, configuration isn't loaded. logger_without_components still rely on configuration, and will create an infinite loop.

Change log entry

None.

Additional Notes:

How to test the change?

CI.

@vpellan vpellan requested a review from a team as a code owner May 9, 2025 14:40
@github-actions github-actions bot added the core Involves Datadog core libraries label May 9, 2025
@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented May 9, 2025

Datadog Report

Branch report: vpellan/fix-logging-during-config-init
Commit report: f824299
Test service: dd-trace-rb

✅ 0 Failed, 21055 Passed, 1375 Skipped, 3m 45.06s Total Time

@pr-commenter
Copy link

pr-commenter bot commented May 9, 2025

Benchmarks

Benchmark execution time: 2025-05-09 15:26:36

Comparing candidate commit f824299 in PR branch vpellan/fix-logging-during-config-init with baseline commit e68b92a in branch master.

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

scenario:line instrumentation - targeted

  • 🟥 throughput [-12858.757op/s; -12398.920op/s] or [-7.680%; -7.405%]

scenario:method instrumentation

  • 🟥 throughput [-11462.135op/s; -10827.042op/s] or [-6.411%; -6.056%]

scenario:profiling - http_transport

  • 🟥 throughput [-970.028op/s; -962.694op/s] or [-21.546%; -21.383%]

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.77%. Comparing base (e68b92a) to head (f824299).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4633   +/-   ##
=======================================
  Coverage   97.77%   97.77%           
=======================================
  Files        1415     1415           
  Lines       86537    86568   +31     
  Branches     4385     4386    +1     
=======================================
+ Hits        84609    84642   +33     
+ Misses       1928     1926    -2     

☔ 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.

@vpellan vpellan merged commit a093c2d into master May 12, 2025
442 checks passed
@vpellan vpellan deleted the vpellan/fix-logging-during-config-init branch May 12, 2025 06:55
@github-actions github-actions bot added this to the 2.16.0 milestone May 12, 2025
@ivoanjo
Copy link
Member

ivoanjo commented May 14, 2025

Hmm I don't think this change is correct.

What other components do to be able to log during initialization is to receive the logger as an argument -- e.g. https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/core/configuration/components.rb#L96-L131 .

Creating a temporary logger makes it impossible to configure by customers (to e.g. hide messages if they don't want to see them). There were actually a few logs from the profiler that used the temporary logger, and I did get customer questions at least twice on how they can be silenced.

@anmarchenko
Copy link
Member

@ivoanjo it already caused some issues for datadog-ci-rb gem that started spewing a lot of debug messages when starting every test session (even without DD_TRACE_DEBUG present), so we need to rethink it before releasing

@vpellan
Copy link
Contributor Author

vpellan commented May 14, 2025

This PR should fix any issue for datadog-ci-rb, and enable configuring it through the env var DD_TRACE_DEBUG

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.

6 participants