instrumentation-jmv - builders miss build() method

[zorba128]

It seems builders for JVM collectors miss build() method. Its only possible to register to the registry.

While I somehow understand the design that there is a separate "metrics" idea (various collector/multicollector incarnations) and registry which is used to - say "consume" them, lack of intermediate grouping possibility causes various trouble.

Like - registry has "unregister" method. How is one supposed to use it, it I'm unable to get reference to collector that exposes group of metrics?
To be specific - how to first register, and then unregister JvmBufferPoolMetrics?

It should be built by registering underlying metrics to form composite collector. Like the PrometheusRegistry, but implementing collector interface. Stripped out of unnecessary concurrency support and mutability.
And such thing can then be passed over to http server or becomes part of more complex infrastructure at higher level.

This seems like duplicate of #903, but I actually have particular problem with this registration/unregistration when trying to upgrade http4s-prometheus-metrics (which wraps prometheus-client-java) from 0.16 to 1.1.

[fstab]

Thanks for bringing this up.

I agree there's currently no way to unregister JvmBufferPoolMetrics. If we want to add support for unregistering, there are multiple ways to implement it:

1. Add an unregister(PrometheusRegistry) method to JvmBufferPoolMetrics that will unregister the three metrics.
2. Add getters for the three metrics so that users can register / unregister them individually.
3. Make JvmBufferPoolMetrics implement MultiCollector.

Based on this ticket I assume you tend towards making JvmBufferPoolMetrics implement MultiCollector. However, there is a drawback to this. If JvmBufferMetrics implements MultiCollector each collect() call needs to create a Snapshots object as return value. That Snapshots object is not needed now as each metric is just registered individually.

So, what do you think of the alternatives?

[zorba128]

Why do you think creation of Snapshots instance is sth wrong? This is just a list...
The code that does all the copying, validations, etc - it should be part of builder, not the data object itself.
And Snapshots.of(snapshot: Snapshot) builder can skip all the validations and copying - no point doing it if you know there is one instance inside. So again I feel you draw some high level business requirements from problematic implementation.