-
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
Telemetry log: Add stack_trace
field when given an exception
#3863
Conversation
BenchmarksBenchmark execution time: 2024-08-29 18:37:05 Comparing candidate commit e76043d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics. |
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.
This is cool! Left a few notes
expect(result).to eq( | ||
[ | ||
'/lib/datadog/core/telemetry/logging.rb:1 in `report`', | ||
'REDACTED', | ||
'REDACTED', | ||
'REDACTED' | ||
].join(',') | ||
) |
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.
Just to doublecheck, does this format comply with the format that the other libraries are using?
We've had a few troubles with telemetry having mismatched formats between libs recently, so I think it's worth being extra careful here.
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.
does this format comply with the format that the other libraries are using?
There is no specification about the format, but it is behaving as how DotNet is sending.
module Logging | ||
extend self | ||
|
||
# Extract datadog stack trace from the exception | ||
module DatadogStackTrace |
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 possibly not for this PR, but it would be cool to perhaps leave stack entries from:
- The Ruby standard library -- paths starting with
RbConfig::CONFIG.fetch("rubylibdir")
(The profiler uses this to categorize Ruby code automatically as being from the standard library!) - dogstatsd-ruby -- by perhaps asking rubygems where dogstatsd-ruby is, when available
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 recommendation for improving the test (I think the current one is too "mocky"), but otherwise it LGTM 👍
89e6ed0
to
0459cfb
Compare
2bab52d
to
e76043d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3863 +/- ##
==========================================
+ Coverage 97.85% 97.86% +0.01%
==========================================
Files 1277 1277
Lines 76364 76394 +30
Branches 3740 3744 +4
==========================================
+ Hits 74723 74761 +38
+ Misses 1641 1633 -8 ☔ View full report in Codecov by Sentry. |
What does this PR do?
Providing a class name of the exception is not very useful. Sending stack_trace help us identified the location is helpful
stack_trace
field with reporting an exceptiondebug
level (The enriched data for debugging can be a risk and high data cardinality)