-
Notifications
You must be signed in to change notification settings - Fork 443
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
ddtrace/tracer: update log msg to accurately count dropped traces #2845
Conversation
BenchmarksBenchmark execution time: 2024-09-24 14:26:09 Comparing candidate commit 214c1bc in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 1 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.
I don't want to block this merge, so I am approving as it looks all clear, but in my opinion and from a troubleshooting perspective, I don't love the approach of "Every X seconds, check how many traces were dropped in the previous interval, and log about it." Logging about the dropped trace at the moment it is dropped is better, but you'd have to go back to using mutexes (or related) to achieve that.
@mtoffl01 we got two requests internally to change the logging because of how spammy it was. Previously, it was indeed emitting a log each time the trace was dropped. So, my suggestion would be to make the interval configurable and smaller, for example 1 second, but avoid logging at each trace drop. What do you think? I can also point you to the original slack threads. |
/merge |
/merge |
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
/merge --cancel |
/merge |
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
🚨 MergeQueue: This merge request is in error mergequeue build completed successfully, but the github api returned an error while merging the pr.
DetailsError: PUT https://api.github.com/repos/DataDog/dd-trace-go/pulls/2845/merge: 405 5 of 5 required status checks are expected. [] FullStacktrace: If you need support, contact us on Slack #devflow with those details! |
/merge |
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
What does this PR do?
debug
.totalTracesDropped
to keep track of the number of dropped traces.Motivation
This PR addresses an issue raised in APMAPI-225. Log messages produced when dropping chunks were too spammy (sometimes occurring > 1,000 times per second) and relayed inaccurate information.
Reviewer's Checklist
Unsure? Have a question? Request a review!