Counter, Histogram, Meter and Timer should be interfaces (in metrics 5.x?)

[mbrannstrom]

Counter, Histogram, Meter and Timer should be interfaces just like Gauge<T>.

Currently it is not that simple to implement a custom Counter (or any other metric), since Counter is a concrete class.

For Gauge you can do:
registry.register(name, (Gauge<Long>)mything::getSomeMetric);

But for Counter you must do:
registry.register(name, new Counter() { @Override public long getCount() { return mything.getSomeMetric(); } });
and yes, we allocate a LongAdder in Counter, which we do not use.

If Counter was an interface, you could have done:
registry.register(name, (Counter)mything::getSomeMetric);

The interfaces Counting and Metric are not that useful, since the MetricRegistry checks instanceof Counter and not Counting (see notifyListenerOfAddedMetric).

This is a breaking change, and might be better suited for metrics 5.x?

[joschi]

But for Counter you must do: registry.register(name, new Counter() { @Override public long getCount() { return mything.getSomeMetric(); } }); and yes, we allocate a LongAdder in Counter, which we do not use.

This looks like a gauge and not like a counter (which is, well, just keeping a count of things).

Could you describe your use case on a more specific example?

[mbrannstrom]

The main difference between counter and gauge are how the are represented when reported, where gauge usually have a value (named "value") and counter have a count (named "count").

A Counter can thus be upgraded to a Meter without affecting consumers of that metric, in a backwards compatible fashion. And with consumers, I mean people writing dashboards in Grafana etc.

Also, while most implementations Counter, Meter, Timer, etc are quite reasonable and well implemented, there are occasions when you already calculate these kind of metrics deep down in your application (without any dropwizard metrics dependencies), and just want to expose those.

So, my use case is that we already have an efficient counter for application logic purposes, and just want to expose that value. I'm already exposing similar values through Timers, so from a consumer perspective the count values should be named "count" in Grafana.

Perhaps the interfaces Counting, Metered, etc should be used more than Counter, Meter, etc? This affects MetricRegistry and listeners.

[vladimir-bukhtoyarov]

@mbrannstrom hello,

Broken backward compatibility is the worst thing for the widely used library. It is better to introduce top-level interfaces and make existed implementations are inherited from new interfaces.

According to the statement that it is hard to extend existing implementations - yes it is awkward but it is not hard(at least for 4.x version). Usually, it is few lines of code for example https://github.com/vladimir-bukhtoyarov/rolling-metrics/blob/master/src/main/java/com/github/rollingmetrics/adapter/GaugeToCounterAdapter.java#L26

Also, we should be thankful that this is possible at all, for example in Spring Micrometer you are not allowed to add custom metrics to registry at all.

[mbrannstrom]

@vladimir-bukhtoyarov OK, apparently there are more people than me that have this use case.
No, it is not hard to do this, but inefficient.

It is better to introduce top-level interfaces and make existed implementations are inherited from new interfaces.

Yes, perhaps so.

Regarding breaking backwards compatibility in the library, something must be done with support for tagged metric names which is currently only present in 5.x. Since 5.x is not final yet, other non-backward changes can be done as well now. Tagged metric names is a deal-breaker for me, and I guess for others as well. Either dropwizard metrics adapt, or is left behind.

[joschi]

@mbrannstrom @vladimir-bukhtoyarov We're a bit short on active contributors to the Dropwizard Metrics project and specifically to the release/5.0.x branch.

Would you be able to suggest the necessary changes in a pull request to the release/5.0.x branch and step up as a regular maintainer (at least for that branch)?

[mbrannstrom]

@joschi Sure.

[mbrannstrom]

Here is an outline to suggested changes.

The metrics supported by the MetricRegistry are:

• interface Gauge<?>
• class Counter implements Counting
• class Meter implements Metered
• class Histogram implements Sampling, Counting, Summing
• class Timer implements Metered, Sampling
  ... plus collections (MetricSet) of these.

All metrics have nice interfaces to support reporters. For example Graphite and InfluxDB reporters already use the fact that both Meter and Timer implements Metered to avoid duplicating code for m1-rate, m5-rate, m15-rate, mean-rate, count and sum. This could be improved further by using only using Counting, Summing, Sampling and Gauge.

if (metric instanceof Gauge<?> g) { 
    writeFieldIfEnabled(MetricAttribute.VALUE, c.getValue()); 
}
if (metric instanceof Counting c) { 
    writeFieldIfEnabled(MetricAttribute.COUNT, c.getCount()); 
}
if (metric instanceof Summing s) { 
    writeFieldIfEnabled(MetricAttribute.SUM, s.getSum()); 
}
if (metric instanceof Metered) {
    writeFieldIfEnabled(M1_RATE, convertRate(meter.getOneMinuteRate()))
}
if (metric instanceof Sampling s) { 
    Snapshot snapshot = s.getSnapshot();
    DoubleUnaryOperator op = metric instanceof Timed ? this::convertDuration : DoubleUnaryOperator.identity();
    writeFieldIfEnabled(MetricAttribute.P99, op.applyAsDouble(snapshot.get99thPercentile()));
    // ... remaining fields
}

Changes:

• Introduce interface Timed implements Metered, Sampling and change class Timer implements Timed.
  • The name "Timed" matches the annotation @timed, in the same way as Metered vs @metered.
• Introduce MetricAttribute VALUE for gauges. InfluxDB reporter already need this.
• Introduce interface Histogrammed implements Sampling, Counting, Summing and change class Histogram implements Histogrammed.
  • The primary use-case is to enable simple code-replacement of "Histogram" with "Histogrammed" in reporters (that are not rewritten according to the suggested pattern above).
  • Anyone that have a better name than "Histogrammed"?
• Change MetricRegistryListener to use interfaces instead of classes, e.g. Timed instead of Timer.
• Change MetricRegistry getTimers() & co to return SortedMap<MetricName, Timed> instead of ..., Timer>

At large, producers of metrics still use classes and e.g. MetricRegistry.counter(..) returns a Counter. However, consumers of metrics use interfaces. This allows a producer do use a custom implementation of these interfaces, such as a custom counter through a lambda:

metricRegistry.register(name, (Counting) mything::getSomeMetric());

Additional changes:
Maybe MetricRegistryListener could be further simplified as:

interface MetricRegistryListener extends EventListener {
    void onMetricAdded(MetricName name, Metric metric);
    void onMetricRemoved(MetricName name, Metric metric); // NOTE: metric value on remove to support demultiplexer adapter below
}

If needed, the old styled demultiplexed listener could be kept (renamed) and an adapter could be implemented:

    void onMetricAdded(MetricName name, Metric metric) {
        if (metric instanceof Gauge<?> g) { delegate.onGaugeAdded(name, g); }
       else if (metric instanceof Counting c) { delegate.onCounterAdded(name, c); }
       // ... other types
    }

What do you think?

[joschi]

@dropwizard/metrics @dropwizard/committers Any opinions on these changes?

[github-actions]

This issue is stale because it has been open 180 days with no activity. Remove the "stale" label or comment or this will be closed in 14 days.