-
Notifications
You must be signed in to change notification settings - Fork 377
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-3210 DI: change logging to be appropriate for customer inspection #4266
Conversation
👋 Hey @p-datadog, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md (possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None. (possible answers No/Nope/None) Visited at: 2025-01-08 15:43:23 UTC |
Datadog ReportBranch report: ✅ 0 Failed, 22132 Passed, 1476 Skipped, 5m 9.15s Total Time |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4266 +/- ##
==========================================
- Coverage 97.73% 97.73% -0.01%
==========================================
Files 1352 1352
Lines 82409 82424 +15
Branches 4224 4232 +8
==========================================
+ Hits 80545 80557 +12
- Misses 1864 1867 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few notes + I had left a few extra text suggestion improvements on the earlier #4230 .
This reverts commit 48a8527.
Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>
I did also apply the wording suggestions, they got reverted but now are reinstated. |
* 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
Replaces #4230
What does this PR do?
Changes logging levels used in DI:
All log messages have
datadog: di:
prefix to make it easy for customers to understand what is generating them.Motivation:
DI currently uses the logger for reporting its internal issues. However, the logger normally logs to customer logs, and as such the default logging should be customer-centric, meaning log entries should only be created for matters that the customers understand and can fix.
Change log entry
Dynamic instrumentation: logging of internal conditions is now done on debug level, and these log entries will not be visible by default in customer applications.
Additional Notes:
I attempted to use block form of logger calls to delay string interpolation until the logging actually happens, but this made my logging assertions fail in the test suite. Since the logging should be very infrequent I kept the calls as they were (pre-interpolated).
How to test the change?
Due to the introduction of an additional layer of abstraction for DI logging, end-to-end tests will need to have additional assertions added for log entries being visible (or not). This will be done as part of DEBUG-3210 but is not part of this PR.