search

eclipse / microprofile-metrics

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

Remove support for metrics from CDI producers #594

Closed jmartisk closed 4 years ago

jmartisk commented 4 years ago

Fixes https://github.com/eclipse/microprofile-metrics/issues/456 @ebullient

ebullient commented 4 years ago

I disagree with this as commented on the issue itself. The reason mentioned in the issue does not convience me as bad CDI practices. If it is dangerous to expose this bean, it is a different matter.

The issue is that @Produces allows metric types to be created that aren’t under the control of the MetricRegistry impl. The implementation should be in complete control of implementation details for all metric types: no arbitrary counter implementations. Ask for a Counter to be injected, and the MP Metrics implementation should be able to construct a counter in whatever way it needs to and provide one.

Gauges are harder, but if we wanted to allow this, I could see having @Produces with @FunctionGauge associated with a Supplier<Number> function that would then be used to register a registry-impl-created Gauge that invokes that function.

@Metric is weirdly not a qualifier, so that isn’t sufficient for @Produces used with @Metric from a “what to inject where” perspective. @Produces @GaugeFunction(...metrics attributes... ) would make more sense from a type safety / expectation perspective.

Emily-Jiang commented 4 years ago

I disagree with this as commented on the issue itself. The reason mentioned in the issue does not convience me as bad CDI practices. If it is dangerous to expose this bean, it is a different matter.

The issue is that @Produces allows metric types to be created that aren’t under the control of the MetricRegistry impl. The implementation should be in complete control of implementation details for all metric types: no arbitrary counter implementations. Ask for a Counter to be injected, and the MP Metrics implementation should be able to construct a counter in whatever way it needs to and provide one.

Gauges are harder, but if we wanted to allow this, I could see having @Produces with @FunctionGauge associated with a Supplier<Number> function that would then be used to register a registry-impl-created Gauge that invokes that function.

@Metric is weirdly not a qualifier, so that isn’t sufficient for @Produces used with @Metric from a “what to inject where” perspective. @Produces @GaugeFunction(...metrics attributes... ) would make more sense from a type safety / expectation perspective.

If you complain about @Metric is not a qualifier, the right fix is to change the annotation by adding @Qualifier instead of deleting the whole annotation.

ebullient commented 4 years ago

If you complain about @Metric is not a qualifier, the right fix is to change the annotation by adding @Qualifier instead of deleting the whole annotation.

@Metric not being a Qualifier is a problem for Produces/Injects for sure. But the general pattern of having the API consumer provide their own metric implementations (either @Produces or the MetricRegistry.register methods) cause problems trying to control how the API behaves (or is adapted onto another implementation, e.g.)

Emily-Jiang commented 4 years ago

After talking with @ebullient, I undstand the big issue is that the supported bean creation is quite dangerous and not useful. I am ok for this to be removed.