TCK does not work with MP Metrics 5.0

[Azquelt]

MP Metrics 5.0 is bringing in breaking changes which require changes in the Fault Tolerance TCK.

The ones that I think affect the Fault Tolerance TCK are:

• @RegistryType is removed, replaced with @RegistryScope
  • @RegistryScope is not a qualifier which means we can't look it up with CDI.current().select()
  • The only way to get hold of the MetricsRegistry for the base scope is to inject it into a bean with the @RegistryScope annotation
• Some methods have been removed from Snapshot, particularly getValues() and getMedian()
  • The median can likely be obtained from the new getPercentileValues() array
  • There's no replacement for getValues() - several of our tests used this to assert that the correct data had been added to the histogram
• Gauge<T> has been changed to Gauge<T extends Number> which breaks binary compatibility because it changes the signatures of methods which return T

I don't think there are any changes needed to the spec or API to accomodate MP Metrics 5.0, just to the TCK.

[Azquelt]

I've opened #610 with the changes needed to support MP Metrics 5.0 only.

It's a little harder to create something that would work on both due to the change in getting hold of the base MetricsRegistry.

You have to use @RegistryScope (which is not a qualifier) on the injection point so there's no way to get it without injecting it so having something that works on both versions of the API is difficult.

Potentially we could add our own copy of @RegistryScope to the TCK and use both @RegistryScope and @RegistryType on the injection point, but then we'd be relying on @RegistryType not being present when the tests are run so that the annotation is ignored.

Another way to do it would be to have separate TCK artifacts for metrics 4.0 and 5.0 which differ only in how they look up the base MetricsRegistry.

[Ladicek]

The fact that @RegistryScope is not a qualifier, yet it's supposed to be used as one, is puzzling. I didn't find any reason in the issues and PRs over at MP Metrics, but I believe it's just wrong.

[Azquelt]

Yeah, I was irked not to find a reason as well. Talking to one of our developers it seems to be because users can now use custom scopes, so you don't know what beans are needed at startup.

Looking at the API, I think you register a custom scope by just injecting its MetricRegistry and using it, so I suspect they could get away with scanning all MetricRegistry injection points at startup, though that would fall apart if they allowed a programmatic way of creating a custom scope.

[Azquelt]

I note that MP Config has a similar issue with @ConfigProperty - there the name is non-binding meaning that all config properties of a particular type are a single bean as far as CDI is concerned. However that does mean that it is a qualifier and I assume (though I haven't checked) that there's a way to get hold of the qualifiers used to select a dependant bean and look at the non-binding attributes, even if it's looked up by CDI.select() rather than injected.

[Ladicek]

Yeah, for MP Config, since the @ConfigProperty-qualified beans are @Dependent, a producer method can inject InjectionPoint and look at the annotation at the injection point.

[Azquelt]

I think that's the idea here too, though you don't actually need the annotation to be a qualifier to examine the InjectionPoint annotations, if you use getMember() or getAnnotated().

[Azquelt]

But I think we're saying the same thing, it would be better if they made it a qualifier, made scope non-binding and used getQualifiers() to find the annotation. Firstly because it is acting as a qualifier so having it be one makes sense and secondly it would make it possible to look up using CDI.current().select().

[Azquelt]

Though if they did that, I think it would still be hard for us to write code which would work on 5.0 without compiling against the 5.0 API since I'm not aware of a way to reflectively create an instance of an annotation.

Maybe we could work around that, if we did write some glue classes compiled against each of the metrics APIs and got all the compiled classes into the TCK jar, I think it would be easier to try to load them via reflection and choose between them based on which Metrics API was available.

[Ladicek]

Personally, I don't really mind if we can't run the TCK with older Metrics implementation.

I just noticed that MetricRegistry instances are singletons per the specification, which is why a producer method can't inject InjectionPoint to lookup a qualifier (that's only possible for @Dependent beans). So there needs to be a more elaborate solution anyway, likely using a portable extension. I suppose the expectation is, as you mentioned above, that a portable extension will scan all injection points of type MetricRegistry, collect the set of scopes, and create the corresponding registry instances. Then, if programmatic lookup was allowed, it would fail with a non-existing scope. Which I think would be fair :-)

[Ladicek]

I see https://github.com/eclipse/microprofile-metrics/issues/746 was filed. I filed https://github.com/eclipse/microprofile-metrics/issues/749 as well.

[Azquelt]

With the merge of https://github.com/eclipse/microprofile-metrics/pull/748, we just need the changes in #612

[Azquelt]

TCK update to add compatibility with Metrics 5.0 released as 4.0.2