-
Notifications
You must be signed in to change notification settings - Fork 375
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
Implement telemetry log #3850
Implement telemetry log #3850
Conversation
0f61ecc
to
fae8d42
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3850 +/- ##
==========================================
+ Coverage 97.83% 97.86% +0.03%
==========================================
Files 1264 1271 +7
Lines 75725 75975 +250
Branches 3729 3739 +10
==========================================
+ Hits 74084 74353 +269
+ Misses 1641 1622 -19 ☔ View full report in Codecov by Sentry. |
BenchmarksBenchmark execution time: 2024-08-21 10:52:26 Comparing candidate commit 5f65370 in PR branch Found 3 performance improvements and 0 performance regressions! Performance is the same for 20 metrics, 2 unstable metrics. scenario:profiler - Allocations (baseline)
scenario:profiler - Allocations (profiling disabled)
scenario:profiler - Allocations (profiling enabled)
|
telemetry.log!(event) | ||
else | ||
Datadog.logger.debug do | ||
"Sending telemetry log about #{message} before telemetry component is ready" |
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.
I find "sending" to be misleading because nothing is actually sent. Suggest either "attempting to send" or "requested to send" or similar. Also, I would put the message at the end so that it's clear where the diagnostics of this component ends and where the forwarded message is. E.g. "Requested to send telemetry log when telemetry component is not ready: #{message}". Is it possible for telemetry to be never turned on, i.e. it would actually not ever become ready?
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.
The telemetry object is always created, even if telemetry is disabled
dd-trace-rb/lib/datadog/core/telemetry/component.rb
Lines 45 to 47 in b7b99b2
Telemetry::Component.new( | |
http_transport: transport, | |
enabled: enabled, |
so it will eventually become no-nil.
I also like that this message is at debug
level, so people have to manually enable debug-level logs to see it.
I do agree that the messaging can be more precise.
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.
Minor: Should we perhaps print the whole event
and not just the message
when logging
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.
I don't think printing the whole event is necessary, since the issue is not caused by event itself.
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.
My thinking for this suggestion is, if later we add more information to the event (e.g. stack), we'll need to remember to tweak the debug message, whereas if we print the whole event, then we won't need to touch it again.
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.
Looks good!
I believe @y9v's and @p-datadog's comments are relevant and should be responded to or addressed.
fae8d42
to
68156a3
Compare
68156a3
to
acbab8c
Compare
becd10d
to
c18946d
Compare
2acf855
to
5ac83a0
Compare
What does this PR do?
This PR enables developer to send telemetry logs for internal observability.