Add possibility to exclude deadlocks & other Hotspot collectors

[hpple]

Hi, @brian-brazil

Here is an alternative approach to fix the issue raised by #612.

[fstab]

Thanks a lot for the PR, I'll get back to this in the next couple of days.

[fstab]

My understanding of the intention of this PR is that users may want to exclude jvm_threads_deadlocked and jvm_threads_deadlocked_monitor for performance reasons. I agree with that, and I think it's a good idea to provide a way to exclude metrics with performance issues.

However, both solutions (#612 and #672) hard-code the assumption that users want to exclude exactly the two metrics jvm_threads_deadlocked and jvm_threads_deadlocked_monitor: #612 by wraps these metrics in an if...else condition that can be disabled, and #672 moves these two metrics to a separate DeadlockExports class that can be excluded.

If a user wants to exclude any other metric except for jvm_threads_deadlocked and jvm_threads_deadlocked_monitor, none of the solutions would help.

I think it would be good to provide a more generic way to exclude metrics by name.

Users could use that generic solution to exclude jvm_threads_deadlocked and jvm_threads_deadlocked_monitor, but if there were other metrics with performance issues, a generic solution would provide a way to exclude them as well.

We already have a similar functionality in CollectorRegistry.filteredMetricFamilySamples(). This is a bit different, because it's per scrape request and not a global setting, but apart from that it achieves a similar goal.

What about extending the CollectorRegistry with an addToExcludeList(String... names) method and use this in the MetricFamilySamplesEnumeration?

Do you think that would solve the issue?

[fstab]

Ok, now I see that in some cases we call Collector.collect() first and filter after that. That won't help with the performance issue. Mabybe we could refactor this and provide an optional Collector.collect(String... excludedNames) so that collectors can apply the filter before metrics are collected.

[hpple]

Hi!

Thank you for your comments!

I suppose I get your point regarding the "hard-code" part of the both approaches.

As far as I understood you'd like to see this "exclude list" with possibility to dynamically reconfigure/extend it on-the-fly (e.g. by introducing the suggested addToExcludeList(String... names)). Otherwise, in case where it will be configured exactly-once (at the moment of creation of a CollectorRegistry) it will be impossible to change this list for the defaultRegistry.

But, frankly speaking, it seems like this flexibility may bring a bit of a confusion.

As far as I can see, the only way to introduce Collector.collect(String... excludedNames) as an optional method (without breaking a compatibility) is to provide the "default" implementation that just delegates to the old one. But it means that you have to change all existing sub-classes to support the filtering feature. Until that, they will work incorrectly. Moreover, you will be doomed to implement this filtering in any future sub-class over and over again.

It does not look like a generic solution for me, unless you're planning to use this approach as an intermediate step and going to deprecate the old collect in favor of the new one.
Please, correct me if I'm wrong about it.

[fstab]

I played a bit with a more generic implementation and created a PR with the current status. It's not complete yet, but I hope it's enough to see how this would look like. The filter is implemented in the MetricNameFilter class. Feel free to provide feedback here #680.

[hpple]

@fstab
Thank you!
That approach looks reasonable and comprehensive.

[fstab]

Closing this as this has been addressed with #680.