Add JvmThreadDeadlockMetrics

[rkurniawati]

Hello,

I was wondering if you would consider accepting the changes in this PR so that we will have these two deadlock-related metrics in a future version of Micrometer?

This PR adds the following JVM thread deadlock-related metrics:

• jvm.threads.deadlocked: The current number of threads that are deadlocked
• jvm.threads.deadlocked.monitor: The current number of threads that are deadlocked on object monitors

Thanks,
Ruth

Ruth Kurniawati

[pivotal-cla]

@rkurniawati Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@rkurniawati Thank you for signing the Contributor License Agreement!

[shakuzen]

Thank you for the pull request. Those sound like interesting additions. I am concerned though that the JavaDoc for both have the following warning (emphasis is mine):

This method is designed for troubleshooting use, but not for synchronization control. It might be an expensive operation.

Due to that it might make more sense to put these in a separate class so users can more easily separately choose to enable these potentially expensive metrics. What do you think? Do you have any practical experience using these in production that you can share in terms of the expense of regularly calling these methods?

[rkurniawati]

Thank you for your feedback, @shakuzen! I agree it's a good idea to let the users enable these metrics when they need them. The APIs used by the deadlock metrics have some overhead in the JVM implementation that I use, calling the findDeadlockedThreads triggers a JVM safepoint which will cause a slowdown that may be significant to some applications.

When you suggest putting these in a separate class, do you mean put them in a separate metric binder? It would be great if you have an example of a metric that's disabled by default.

Thanks,
Ruth

[shakuzen]

When you suggest putting these in a separate class, do you mean put them in a separate metric binder?

Yes. By doing that, users can bind JvmThreadMetrics without these potentially heavy metrics being registered. When users want these heavy metrics, they should make the choice to bind the new binder containing them. It's up to users. Everything is disabled by default in the sense that no meters are registered until you bind a MeterBinder or register a meter.

[rkurniawati]

Thanks! I refactored the code and put the new metrics in a class called JvmThreadDeadlockMetrics. I've verified that these metrics are not enabled by Spring Boot's auto-configuration, but can be enabled by binding the JvmDeadlockedThreadMetrics binder to the registry.

Let me know if there's anything else that I could do to improve this PR 😃

[shakuzen]

Thank you for the changes. I left some review comments.

[shakuzen]

Could we control this with something like CountDownLatch so we can have the same effect without the test always taking multiple seconds?

[rkurniawati]

I changed the test to use CountDownLatch. Unfortunately I have to use ReentrantLock instead of the monitor lock, so the the jvm.threads.deadlocked is non-zero but jvm.threads.deadlocked.monitor is zero

[rkurniawati]

It looks like the CI job failed due to a flaky test (##5296). I'm going to try to rebase so that it will trigger CI (and hopefully pass the test next time)

[shakuzen]

Thank you for the quick updates. One more thing and then this is ready to go I think.

[shakuzen]

Thank you for the pull request and the patience on review.

[shakuzen]

This should be isSynchronizerUsageSupported. I'll fix it in a polish commit.

[rkurniawati]

Oops! Thanks for taking care of it!