search

eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

`@RegistryScope` should be a qualifier #749

Closed Ladicek closed 10 months ago

Ladicek commented 1 year ago

@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 https://github.com/eclipse/microprofile-fault-tolerance/issues/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 1 year ago

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 1 year ago

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 1 year ago

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).