-
Notifications
You must be signed in to change notification settings - Fork 772
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
Added defensive code for scenario where thread id <= 0 #784
Conversation
PR to resolve #784 which will ultimately resolves prometheus/jmx_exporter#601 @fstab please review |
d343292
to
ed7b3b0
Compare
Hi Doug, thanks a lot for the PR. Could you elaborate what this is about? As far as I can see From the Javadoc of
So I'm wondering where non-positive thread IDs could come from? |
TL;DR ZooKeeper class https://issues.apache.org/jira/browse/ZOOKEEPER-4460 I feel that it shouldn't break Detailed Analysis ThreadExports Test Case
Test Case Result (without the fix)
Update: Added the test case to |
6c700de
to
62625d6
Compare
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 a lot for the explanation. That's unexpected. I agree we should handle this and not just fail in that case. I left a few review comments. Thanks for picking this up!
simpleclient_hotspot/src/main/java/io/prometheus/client/hotspot/ThreadExports.java
Outdated
Show resolved
Hide resolved
simpleclient_httpserver/src/main/java/io/prometheus/client/exporter/HTTPServer.java
Outdated
Show resolved
Hide resolved
c965466
to
8f7206f
Compare
Signed-off-by: Doug Hoard <doug.hoard@gmail.com>
Thank you! |
Signed-off-by: Doug Hoard doug.hoard@gmail.com