Provides extension point to access to individual measurements

[mrmanc]

As suggested in #595.

I’ve done this work on the 3.2-development branch as it seemed most up to date. I’d be happy to move it to 4.0-development as it is a breaking change to the public API of Metric.

• MetricRegistry.addMeasurementListener() now provides an extension point which lets users add listeners which will receive individual measurements for all registered metrics.
• The registry holds a single MeasurementPublisher so that each metric object does not need to hold a separate collection.
• Adds the metric name and a publisher to the signature of all metric constructors to allow them to emit measurements with the associated name. I couldn’t see a nicer simple way of doing this while retaining the metrics as the source of measurements.
• Nested metrics do not report their measurements since these are meta-metrics—aggregations themselves.
• There is now a slightly wrinkly problem around MetricRegistry.register() since the name of the metric is supplied and present in the metric. I could add a name() accessor to Metric but this would be a further break to the interface.
• Gauge measurements do not make it to the listeners. I can contribute an instance of ScheduledReporter which optionally passes (configured through the MetricRegistry) Gauge samples on a regular interval to listeners, and will contribute this approach if the maintainers are happy.

[vladimir-bukhtoyarov]

I see following problems with this PR:

• All third-party implementations of histograms, timers and counters will be broken by this change.
• All client code which instantiates any metric directly through constructor will be broken.
• The metric name must not be a private field of metric itself, because one metric can be added to multiple registers, and same metric can has different names in different registers.

Also the problem of listeners can be easy solved by using decorator pattern, without adding unnecessary complicity to the core of library.

[mrmanc]

Thanks for the comments!

I have gone back and forth on the design of this change, trading off complexity in the design versus breaking existing code and felt that actually submitting something might lead to some good suggestions.

All third-party implementations of histograms, timers and counters will be broken by this change.

I’m not sure that third party implementations histograms, timers and counters would be broken unless you’re referring to the change to MetricSupplier?

All client code which instantiates any metric directly through constructor will be broken.

What do you think about retaining the previous constructors in a deprecated form? That way no client code would break, but measurements would be lost for code which wasn’t updated.

The metric name must not be a private field of metric itself, because one metric can be added to multiple registers, and same metric can has different names in different registers.

That’s interesting—I wondered if there was an explicit design decision behind the name not existing on the metrics themselves and hadn’t considered that. Our more complicated design for this change gets around having to add the name to the metrics by holding it on an intermediate object which acts as a listener on the metric. It’s a layer of complication I wanted to avoid but could reintroduce to resolve that problem.

Also the problem of listeners can be easy solved by using decorator pattern, without adding unnecessary complicity to the core of library.

Are you suggesting decorating the registry rather than changing any classes in the library itself? Following that approach, I could not see a way to obtain measurements emitted by third party client code. I believe a change to each implementation of Metric would still be required, although I believe that can be done without breaking any public API.

[clohfink]

Why not just make a custom reporter that pushes the metric to the listeners? Then there wont be any broken apis and no changes needed in metrics core.

[ryantenney]

Something resembling this is planned for v4. Let me take a look at it tomorrow and I'll see if it's a good fit.

…

On Sat, Feb 25, 2017 at 6:11 PM Chris Lohfink ***@***.***> wrote: Why not just make a custom reporter that pushes the metric to the listeners? Then there wont be any broken apis and no changes needed in metrics core. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1091 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHkbyHprQuERJQcFoiuX8xpvbvSzT7kks5rgLU2gaJpZM4MMJZ7> .

[mrmanc]

@clohfink

Why not just make a custom reporter that pushes the metric to the listeners? Then there wont be any broken apis and no changes needed in metrics core.

Using a reporter would send an aggregate of the measurements. This change is intended to make it possible to perform aggregation elsewhere so that it’s not a double aggregation. For example, if we wanted the 99th percentile of a timer, we can either take an average of the 99th percentile across each node as sent by a reporter, or we can take the 99th percentile across all of the values. These can have significantly different results when measurements are not evenly balanced across nodes.

@ryantenney I totally missed that you had since opened #822 until a few minutes ago. I hadn’t seen any activity around this so thought I’d contribute my thoughts. Thanks for the confirmation that it’s still planned for v4.

[vladimir-bukhtoyarov]

@mrmanc,

I’m not sure that third party implementations histograms, timers and counters would be broken unless you’re referring to the change to MetricSupplier?

There are many open-source projects like Cassandra or Rabbitmq Java Driver uses Dropwizzard/Metrics for monitoring and makes own implementation of timers or histograms. In general Metrics Core is well extensible, but absence of interfaces(only Gauge has interface) for metrics is worst drawback for extensibility. As result, there is only one way to provide own implementation would be subclassing already existed class from core. But subclass must call constructor of superclass and if already used constructor changes its signature then subclass will be broken. So each client which uses this kind of third-party libraries will decline migration to new version of metrics-core until thirdparty library will be compatible with new versions of metrics-core, and this creates the risks of community splitting.

What do you think about retaining the previous constructors in a deprecated form? That way no client code would break, but measurements would be lost for code which wasn’t updated.

This solution help to preserve backward compatibility, but introduces the point of inconsistency. Your request is heavy concentrated on flow when MetricRegistry creates the new Metric instance, but there is another flow when Metric firstly instantiated in client code through constructor and then added to registry, and that is why specifying listeners at construction time is undesirable, because code which creates new metric potentially is very far from MetricsRegistry and knows nothing about listeners which can be applied on metric.

[mrmanc]

@vladimir-bukhtoyarov

There are many open-source projects like Cassandra or Rabbitmq Java Driver uses Dropwizzard/Metrics for monitoring and makes own implementation of timers or histograms.
…
As result, there is only one way to provide own implementation would be subclassing already existed class from core. But subclass must call constructor of superclass and if already used constructor changes its signature then subclass will be broken.

Yep, I see that now, thanks for clarifying. Extracting interfaces from each of these implementations would also break downstream implementations but would be easier to resolve. Could you point me at those implementations you mentioned as I was unable to find them? The implementations I did find appeared to exist to alter the aggregation mechanism or to hook into the measurements. The latter case should be satisfied by providing a mechanism to hook into measurements.

I think the first case highlights a problem with the current design. The identity of the metrics and mechanism to report a measurement is too closely coupled to the metrics aggregation. That reminds me of a comment made by @bentatham in #822 about making the current aggregations observers themselves.

Metrics has become somewhat a de-facto standard around recording metrics, and I think that has been positive, encouraging more third party instrumentation of code with correct semantics around the type of events. I think I would love to see more separation between the interface (to register, retrieve and measure against metrics) and the rest of the functionality. This would allow people to make things ‘metrics-compatible’ without making any assumptions about what happens downstream.

So each client which uses this kind of third-party libraries will decline migration to new version of metrics-core until thirdparty library will be compatible with new versions of metrics-core, and this creates the risks of community splitting.

Since 4.0 is going to be a breaking change as it is then third party library updates seem like they are going to be necessary anyway.

This solution help to preserve backward compatibility, but introduces the point of inconsistency. Your request is heavy concentrated on flow when MetricRegistry creates the new Metric instance, but there is another flow when Metric firstly instantiated in client code through constructor and then added to registry, and that is why specifying listeners at construction time is undesirable, because code which creates new metric potentially is very far from MetricsRegistry and knows nothing about listeners which can be applied on metric.

Agreed. How would you feel about making the implementations mutable by adding an addMeasurementListener() method instead? Mutability is something I try to avoid.

[vladimir-bukhtoyarov]

How would you feel about making the implementations mutable by adding an addMeasurementListener() method instead?

@mrmanc I think it would be better to not change existing Metric implementations and MetricRegistry. You can easy achieve your goals by extending registry and metrics, else there is no clean and effective way to keep API and documentation in consistency.