[PROF-11045] Fix profiling warnings being really hard to silence #4232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since most folks are not using ractors, this is more of a "FYI/BTW". Also, I've slightly tweaked the text to reduce the overall string, while keeping the same info.
…est suite execution
Receiving the logger as argument means that we'll use the latest logger configured via `Datadog.configure`; in contrast with using the `Datadog.logger` directly, since during components initialization the **older/fallback** logger gets used, which won't reflect any settings given. The distinction is subtle but important: consider the case of setting log level. Prior to this PR, the log level got ignored by the `Profiling::Component` since the old/fallback logger usually did not pick itt up. After this PR, enabling profiling and changing the log level in the same `Datadog.configure` block will produce the expected effect of Profiling::Component respecting that log level.
We had left these TODOs a while ago and since I'm cleaning up a number of logging-related things in the profiling component, I decided to go ahead and clear these ones as well.
This makes sure these information messages, when printed, are not printed more than once. I considered what to do with the warning messages as well, but since having individual OnlyOnce instances is quite a bit of boilerplate for the number of warnings we are emitting, I decided to target only this one for now.
github-actions
bot
added
core
ivoanjo
added
bug
ivoanjo
commented
Dec 17, 2024
Datadog ReportBranch report: ✅ 0 Failed, 22311 Passed, 1476 Skipped, 5m 35.67s Total Time |
These regressions are false positives, I'll see what I can do about updating our config to correct this. |
BenchmarksBenchmark execution time: 2024-12-17 14:38:21 Comparing candidate commit 12ca619 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4232 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 1355 1355
Lines 82145 82149 +4
Branches 4216 4216
=======================================
+ Hits 80301 80305 +4
Misses 1844 1844 ☔ View full report in Codecov by Sentry. |
p-datadog
approved these changes
Dec 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR fixes the report we had in #4231 that the profiling initialization warnings were hard to silence.
In particular, due to the way the profiling component was accessing the dd-trace-rb logger, any configuration changes to change the log level were actually not visible to the profiling component. This issue is now fixed, and enabling profiling + changing the log level in the same
Datadog.configureblock will be respected.I also included a few more improvements:
OnlyOnceso even when it does get logged, it will only get logged at most onceMotivation:
Fixes #4231
Change log entry
Yes. Fix profiling warnings being really hard to silence
Additional Notes:
There's still one case where it's not possible to avoid the warning: when starting profiling via an environment variable (e.g.
DD_PROFILING_ENABLED=true bundle exec ddprofrb ruby <somescript>).This is a result of there not being a way to set the log level of dd-trace-rb via an environment variable. I decided to not tackle this one for now; hopefully the fixes above are enough of an improvement to the status quo ;)
How to test the change?
Here's two sample test cases:
DD_PROFILING_ENABLED=true DD_PROFILING_ALLOCATION_ENABLED=true bundle exec ddprofrb exec ruby -e "Datadog.configure { |c| c.logger.level = Logger::ERROR; c.profiling.enabled = true };"With this PR prints the "using Ractors may result in allocation profiling stopping" only once, whereas in master it prints it multiple times
DD_PROFILING_ALLOCATION_ENABLED=true bundle exec ruby -e "require 'datadog/auto_instrument'; Datadog.configure { |c| c.logger.level = Logger::ERROR; c.profiling.enabled = true }"With this PR does not print the message; whereas in master it prints it once.