-
Notifications
You must be signed in to change notification settings - Fork 482
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
Track dropped spans and logs due to full buffer #2357
Track dropped spans and logs due to full buffer #2357
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2357 +/- ##
=======================================
- Coverage 79.5% 79.5% -0.1%
=======================================
Files 123 123
Lines 21448 21482 +34
=======================================
+ Hits 17061 17087 +26
- Misses 4387 4395 +8 ☔ 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.
Thanks for working on this! Left my comments in the PR.
OTel internal metrics is something we can add, but not right away:
https://github.com/open-telemetry/opentelemetry-rust/pull/2357/files#r1860843464
dropped_logs_count: AtomicUsize, | ||
|
||
// Track the maximum queue size that was configured for this processor | ||
max_queue_size: usize, |
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.
odd that we have to store this here just for logging purposes, but not an issue!
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 believe we can avoid this by moving the logging of dropped logs (otel_warn!) from the shutdown() method to the worker's Shutdown message processing. Also, dropped_logs_count
to be shared with the shutdown worker and the processor object. Haven't tried, but if it seems to be complex, we can park it for separate PR.
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.
Thanks!
Left some suggestions for improved log messages.
Co-authored-by: Cijo Thomas <[email protected]>
Changes applied - should be good to go once the builds are done. |
Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
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 suggestion to be consistent with the message content for both the batch processor.
Would be good to include a unit test to validate that the dropped logs count is correctly reflected during shutdown. Since the field is private, we could consider adding a test-only accessor method to retrieve it. That said, if this is time-sensitive and needs to be included in today's release, can be done separately. |
I think we have orchestrated such a test in OTel .NET, so we can copy some ideas. |
@scottgerring I edited the PR Title ("chore: Track dropped spans and logs due to full buffer"), to remove "chore" from it. |
Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]> Co-authored-by: Cijo Thomas <[email protected]>
Fixes #2273
Design discussion issue (if applicable) #
Changes
As suggested in #2273, this change adds a counter and a metric for dropped spans and logs; the counter is used to log total drop count at exporter shutdown, the metric is used to export this information.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes