New methods to create Gauges in 3.0 are too implementation dependent

[ebullient]

https://github.com/eclipse/microprofile-metrics/blob/c54cf5592e4193e6b4fb50b12f6db5641d947d91/api/src/main/java/org/eclipse/microprofile/metrics/MetricRegistry.java#L314

The new signatures for Gauges require pre-created Metric implementations (which causes problems when you want to adapt one API onto another).

Instead of this: Gauge<?> gauge(String name, Gauge<?> gauge, Tag...tags);

Use something like this (as Micrometer does):

Gauge gauge(String name, Object o, ToDoubleFunction f, Tag...tags); Gauge gauge(String name, Supplier f, Tag...tags);

Either of these can be used to allow a registry impl to construct their own Gauge that monitors the provided function.

[jmartisk]

The register methods also require pre-created implementations (for all metric types, not just gauges) and have been there for a long time, isn't that a problem too?

We could surely add variants like

Gauge<T> gauge(String name, Supplier<T> supplier, Tag... tags);

and expect the implementation to wrap the supplier into a Gauge<T>

But for consistency it feels like we should keep the old ones too, or is that bad?

[jmartisk]

Btw time for 3.0 is slowly running out and we're all quite busy, so a submitted PR would be most welcome :)

[ebullient]

The register methods also require pre-created implementations (for all metric types, not just gauges) and have been there for a long time, isn't that a problem too?

yep.

We could surely add variants like

Gauge<T> gauge(String name, Supplier<T> supplier, Tag... tags);

and expect the implementation to wrap the supplier into a Gauge<T>

But for consistency it feels like we should keep the old ones too, or is that bad?

I don’t know. I ended up having to use some of the old register methods in my latest attempts at refactoring hibernate-orm and agroal. Micrometer has a FunctionCounter (with similar behavior to Gauge). It may be that adding a counter method that accepts supplier number, or alternately adding a builder on Counter that accepts more variety (where the Builder has a register method on it) might allow this without parameter explosion on MetricRegistry.

[donbourne]

Discussed in the call today that we will try to add the 2 new suggested gauge methods and remove the Gauge<?> gauge(String name, Gauge<?> gauge); method.

@ebullient , we need more info on what we should do about the register methods. Quarkus and Open Liberty register "pseudo-counters" in places where the spec requires a counter to be used for a particular metric but where the impl cannot directly instrument the component that needs to be monitored. If register methods are going to continue to allow foreign Counters to be registered using the register method, are you any closer to solving your problem?

[ebullient]

I've made something that works to abstract between the two systems that relies exclusively on Supplier and ToDoubleFunction to remove requirement to have/use a thing of a certain type.

Abstracted API is here: https://github.com/ebullient/quarkus/blob/metrics/core/runtime/src/main/java/io/quarkus/runtime/metrics/MetricsFactory.java

SmallRye impl is here (note mapping to counter or gauge to satisfy the MP Metrics API): https://github.com/ebullient/quarkus/blob/metrics/extensions/smallrye-metrics/runtime/src/main/java/io/quarkus/smallrye/metrics/runtime/SmallRyeMetricsFactory.java

Equivalent Factory impl for Micrometer looks like this: https://github.com/ebullient/quarkus-micrometer-extension/blob/experimental/runtime/src/main/java/dev/ebullient/micrometer/runtime/MicrometerMetricsFactory.java

That at least allowed me to map between the systems w/o so many glue objects in between. So in this case, the register methods of foreign objects can/should(?) all be replaced with Supplier or ToDoubleFunction patterns.. which gives you something to register w/o losing control of what is measured (also makes Counters, Gauges, etc. consistent). Timers get tricky, but I've made allowances for that as well (at least at a basic level for what I need).

[jkschneider]

If it helps clarity a bit, a FunctionCounter should have a similar API surface area as a Gauge, but the user is responsible for promising to meet the contract that the observable value is monotonically increasing.

[jmartisk]

Thinking what would be the potential impact of removing the register methods.

Surely users/implementations would be able to switch from calling register(MyOwnCounterImplementation) to calling counter(Supplier) that we'd add, and likewise for gauges.

I'm not sure about other metric types though, for example for Timer, SmallRye allows setting a custom Clock and Reservoir when creating the TimerImpl (see https://github.com/smallrye/smallrye-metrics/blob/2.4.2/implementation/src/main/java/io/smallrye/metrics/app/TimerImpl.java#L73). This doesn't sound like something we can cover using neutral Java interfaces, but I don't have any idea whether someone is really making use of this. AFAIK OpenLiberty allows the same so maybe the IBM guys have got ideas whether we can afford to lose this.

[ebullient]

Thinking what would be the potential impact of removing the register methods.

I'm not sure about other metric types though, for example for Timer, SmallRye allows setting a custom Clock and Reservoir when creating the TimerImpl (see https://github.com/smallrye/smallrye-metrics/blob/2.4.2/implementation/src/main/java/io/smallrye/metrics/app/TimerImpl.java#L73). This doesn't sound like something we can cover using neutral Java interfaces, but I don't have any idea whether someone is really making use of this. AFAIK OpenLiberty allows the same so maybe the IBM guys have got ideas whether we can afford to lose this.

I suppose it depends on how/where this is configured. I know I am really biased by how micrometer works.. it supports separate creation of config that can be applied by name. It also specifies the Clock as registry config, rather than as config for the individual timer.

@jkschneider -- do you have an opinion re: Reservior?

[ebullient]

Some notes on reservoirs: https://medium.com/expedia-group-tech/your-latency-metrics-could-be-misleading-you-how-hdrhistogram-can-help-9d545b598374 https://groups.google.com/forum/#!msg/mechanical-sympathy/I4JfZQ1GYi8/ocuzIyC3N9EJ

[donbourne]

@Channyboy , this is fixed by #598 now, right?

[Channyboy]

Yup. Closing. Any further concerns can be brought up in a new issue.