MetricRegistry needs API improvments

[jbee]

Hi,

noticed that MetricRegistry is odd to use and implement which mostly is an performance issue as logical lookups are likely to create intermediate collections.

In particular there is no general key based lookup for Metric (type unknown/unspecified) so that only way to access is getMetrics().get(...). Exposing all metrics as Map is unfortunate and somewhat contradictory to the existence of methods like getCounters(). It would be better to have a more restricted API with lookup methods only and some way to iterate or stream process "all" metrics. Exposing them as Map forces the implementation to either make heavy use of views or create intermediate collections. I hope my suggestions below also support that getMetrics() and getMetadata() should be deprecated and removed.

Suggestions

Following Metric lookup methods would be helpful:

Metric getMetric(MetricID key);
// or
<T extends Metric> T getMetric(MetricID key, Class<T> type) throws IllegalArgumentException;
// also
Metadata getMetadata(String name);
// and
SortedMap<MetricID, Metric> getMetrics(MetricFilter filter);

The above methods could be used to implement getCounters(MetricFilter filter) and alike as default methods. This would require some ugly casts but its still "type safe".

Also there are no key based lookup methods that accept MetricID as key.

default Counter getCounter(MetricID key) {
  return getCounters((id, metric) -> id.equals(key)).get(key);
}
// ... and so forth

A smaller but similar problem exists with the find or create methods like counter. The API contains no variant that accepts MetricID even though this should be the main one that actually needs to be implemented while the others could be default methods calling this one. I suggest to add:

Counter counter(MetricID id);
// ...

and implement the existing variants as default methods:

default Counter counter(String name) {
  return counter(new MetricID(name));
}
default Counter counter(String name, Tag... tags) {
  return counter(new MetricID(name, tags));
}
//...

In addition since many methods in the API accept Tags in form of Tag[] it makes sense that MetricID has a method:

  Tag[] getTagsAsArray();

[jbee]

Bonus request: Current API does not offer a thread-safe way to get or create a Gauge. Only access is getGauges() but even if that does not contain the gauge we otherwise want to create by the time we do create the gauge using register it might already exist at which point an exception is thrown.

Why create gauges that might already exist? For example the MP FT metrics where (depending on the FT annotation) a gauge might be created for an annotated method. Each time FT for that method is processed we try to make sure we have connected the gauge to the value provider.

[jmartisk]

Hi @jbee , so I'll try to break it down into separate suggestions and state my opinions on them:

`Metric getMetric(MetricID key)`
`Metadata getMetadata(String name)`
`SortedMap<MetricID, Metric> getMetrics(MetricFilter filter)`

• These make sense to me and I think we could do it.

<T extends Metric> T getMetric(MetricID key, Class<T> type) throws IllegalArgumentException

• I would find this a little problematic because of the type parameter T. What exactly should the allowed values of T be? Just the metric interfaces (Counter, Histogram, etc.) or including implementation classes? If we accepted implementation classes, it might not be able to safely determine the metric type in case that an implementation (or even an application) chooses to have an implementation class that implements multiple metric types at once, which is legal.

getCounter(MetricID key) and the same for other metric types

• This does make sense, except we can't make them default because MetricRegistry is an abstract class, not an interface. We could turn them into concrete methods so that the implementations don't have to supply their own implementation if they don't want to, but I would probably prefer keeping them abstract, because we're trying to minimize the amount of program logic in the API code, it should really be kept as API only if possible.

counter(MetricID key) and the same for other metric types

• Yeah, actually I have been thinking the same thing, this would be useful. Let's just leave the existing variants unchanged, for the same reason I explained above.

Regarding the Gauge registration, the problem is deciding what to do when a gauge is already registered, and someone is trying to re-register a different object that implements Gauge - we could either keep the old one, or replace it with the new one, both scenarios make sense under some circumstances. So, that might mean we need to introduce

public abstract Gauge<?> gauge(String name, Gauge<?> gauge, boolean replaceExisting);
public abstract Gauge<?> gauge(String name, Tag[] tags, Gauge<?> gauge, boolean replaceExisting);
public abstract Gauge<?> gauge(Metadata metadata, Gauge<?> gauge, boolean replaceExisting);
public abstract Gauge<?> gauge(Metadata metadata, Tag[] tags, Gauge<?> gauge, boolean replaceExisting);

these methods would not throw an exception if a gauge already exists - it would possibly get replaced, and the methods would return the "new" gauge if replacing happened, or the "old" one if the old one was kept.

What do you think? That's quite a lot of new methods, but I kinda agree with your concerns, they would be helpful. By the way if you want to come up with a PR, that would be more than welcome ;)

[jbee]

I might have some room next week to get a PR going.

we can't make them default because MetricRegistry is an abstract class, not an interface

Looks to me there is nothing preventing MetricRegistry being an interface. Why isn't it? Pre J8 compatibility?

because we're trying to minimize the amount of program logic in the API code

Especially with the variants of providing name + tags there is only one correct way to implement it. Letting each implementation duplicate it feels like a job creation scheme. It also raises the question for the implementer why this had not been solved already on the abstract level.

<T extends Metric> T getMetric(MetricID key, Class<T> type) throws IllegalArgumentException

What exactly should the allowed values of T be?

There is no problem here. It is purely for convenience and would be implemented as follows:

<T extends Metric> T getMetric(MetricID key, Class<T> type) throws IllegalArgumentException {
  Metric result =  getMetric(key);
  try {
    return type.cast(result);
  } catch (ClassCastException e) {
    throw new IllegalArgumentException("Some description", e);
  }
}

So any type the caller is willing to expect is valid. It does not matter if a class implements multiple interfaces - it only matters what the caller asks for and if that works out. The bound of <T extends Metric> is just to keep usage sane and give the user a hint how this is supposed to be used.

Regarding the Gauge registration, the problem is deciding what to do when a gauge is already registered.

What I had in mind is a get or register. If a gauge for the given MetricID exists it is a get, so the existing gauge is returned. If not it is a register with all existing rules applied. The only reason this is a good idea to have instead of implementing it as...

Gauge g = registry.getMetric(gaugeID, Gauge.class);
if (g == null) {
  g = registry.register(gaugeID, createGauage(...));
}

...is, that the above cannot be atomic and could fail at the register call because in the meantime the gauge does exist. Having this as a API method allows the implementation to take care of proper synchronisation and either return an existing instance or the newly created one and not potentially fail with an IllegalArgumentException. The replaceExisting flag is yet another use case which I feel does not fit into how semantics of the registry are so far. It always is "once defined it cannot be changed". Breaking this for gauges would be odd.

That's quite a lot of new methods...

Yes, so I also suggest to remove:

    public abstract Map<MetricID, Metric> getMetrics();

    public abstract Map<String, Metadata> getMetadata();

With the new methods this can be done using getMetric(MetricID) and getMetadata(String). And there already is getNames() and getMetricIDs(). This lets you do everything you could do with the Maps but for the most common case of accessing a single Metric by its MetricID or Metadata by its String name no intermediate collection would be created. I say intermediate collection because in order to implement the registry in a thread-safe way it would be a very bad idea to organise the data in a map of metrics and a map of metadata internally as this would force almost all methods to be synchronized. To benefit from concurrent collections the organisation needs to be different which means the two methods either need to use a view (lots of implementation work) or return computed intermediate collections (little code, lots of overhead, especially if all we wanted was a single value from that map).