eclipse/microprofile-metrics

`@RegistryScope` should be a qualifier

Ladicek opened this issue · 3 comments

Ladicek commented

@RegistryScope is supposed to be used as a qualifier (that is, on injection points of MetricRegistry to select which specific instance should be injected), but is not a @Qualifier. In my opinion, that is wrong per se, but also because it disallows programmatic lookup of a[n existing] metric registry.

Based on the discussion in eclipse/microprofile-fault-tolerance#609, it seems this was intentional, because making @RegistryScope a qualifier would enable programmatic lookup, which would make the set of metric registries open ended, but I don't necessarily agree with that reasoning. It would in my opinion be perfectly fair for a programmatic lookup of a registry instance with previously-undeclared scope to fail.

comment from @donbourne :
once we complete this issue, we should open an issue for MP FT to switch to using RegistryScope in their TCK, instead of RegistryType.

donbourne commented

It would in my opinion be perfectly fair for a programmatic lookup of a registry instance with previously-undeclared scope to fail.

The intent for RegistryScope is that there are 2 ways to create a new (custom) registry:

  1. by specifying a scope as part of a metric annotation where the value indicates the new registry name
  2. by injecting a MetricRegistry with a RegistryScope indicating the new registry name

If we were to eliminate the second way then there would be no way of creating a new MetricRegistry scope for use with non-annotation-based metrics.

Ladicek commented

I don't suggest at all to eliminate the 2nd option and I don't think making @RegistryScope a qualifier would require that.

donbourne commented

thanks @Ladicek , I get what you mean now. That makes sense and I agree it would be better to have it be a qualifier. Given the short time remaining we may or may not get that resolved in time for 5.0. In the meantime we're looking at putting back RegistryType as a (deprecated) qualifier to use with MetricRegistry instances.

If we don't get this done in time for 5.0 we should be able to make this change for 5.1 (backwards compatible).