Issues with support for CDI producers

[jmartisk]

The spec (https://github.com/eclipse/microprofile-metrics/blob/2.1.0/spec/src/main/asciidoc/app-programming-model.adoc#field) says that when a producer method/field is annotated with @Metric, a corresponding metric must get registered.

Example:

    @Produces
    @Metric(name = "counter-from-method",
            tags = {"a=b", "c=d"}
    )
    @ApplicationScoped
    public Counter getCounter() {
        return new CounterImpl();        
    }

The spec also requires that it must be possible to inject metrics like this:

@Inject
@Metric(name = "applicationCount")
Counter count;

This has some consequences: Problem 1: The injection part can be only achieved by there being a CDI producer in the Metrics implementation code that produces instances of this particular metric type. This means that if a developer creates another CDI producer in the application code, like in the example at the beginning, from CDI perspective the injection point will be ambiguous (both the producer in the application and in the Metrics implementation will be eligible), unless they annotate their producer and injection point with some custom qualifiers. The specification doesn't mention that such producer will exist, but I suppose it's the only way to implement this.

If the implementation declares a @Priority on the producer and the application's producer has a higher priority, this problem is partially resolved (the ambiguity), but one loses the possibility to use the original "smart" producer from the implementation that knows how to inject the right metric depending on the metadata provided in the @Metric annotation.

Problem 2: What if the producer declares a different scope than @ApplicationScoped or @Singleton? In that case, it will create multiple metric objects over time. Which one should be the one to be registered as counter-from-method? The spec doesn't say anything about this. If we, instead of storing one particular instance directly, store a CDI contextual reference in the registry (which is what I believe most implementations currently do), then the contextual reference will point at different metric objects depending on the active scope - for example, for a metric from a @RequestScoped producer, each localhost:8080/metrics request will export data from a newly created counter just for this request! This does not make any sense. Or even worse, for some kinds of scopes, the scope may not even be active at all. I think that metric producers should be required to be @Singleton or @ApplicationScoped if we really want to automatically create metric registrations from them.

Producing various metrics depending on the context can be a valid use case (for example you could have a producer that retrieves a metric for designated for the current user, transaction, etc) but it is a nonsense to register the producer as one entry in the registry if it produces different metric objects over time. In such case, the metric object for each scope (user/transaction/etc) should have its own separate registration.

My suggestion what to do:

• Mention in the spec that developers might need to use qualifiers to resolve the ambiguity that there will probably be if they declare producers of metrics.
• Decide whether a @Produces field/method annotated with @Metric should really trigger a metric registration - I think not. And if we really need to have it, then it should be performed only for @ApplicationScoped or @Singleton-scoped producers.

[donbourne]

@jmartisk I think we need to look at how implementations are handling the second problem today. For example, when you have an @Counted annotation on a request scoped JAX-RS resource, I expect that nobody is creating/registering separate instances of the metric for each request. (Perhaps that is bad practice for CDI, but creating separate metric instances for each request is useless.)

[jmartisk]

The spec currently says

Depending on CDI bean scope, there may be multiple instances of the CDI bean created over the lifecycle of an application. Metrics, other than gauges, declared using annotations on CDI beans may therefore also have multiple instances. In these cases, where multiple metric instances exist corresponding to the instances of the CDI bean, updates to the value of the metric will be combined. For example, calls to a method annotated with @Counted will increase the value of the same counter no matter which bean instance is the one where the counted method is being invoked.

I think this might not be the best wording - it's useless to create multiple metrics instances. The best way to do this is to simply register one instance and then the interceptor will always retrieve it from the registry and update it during a method call. Perhaps we could change this description, WDYT?

But this concerns annotated methods only. Producers are a bit different because at one point we have to call the producer (which is application code) to obtain a metric instance, whereas with annotated methods, the metrics are instantiated directly by the Metrics implementation.

Regarding what happens in producers, SmallRye and its downstream dependents do it as I described, a CDI contextual reference is stored in the MetricRegistry during container initialization. It's not ideal and for non-application scoped beans this means funny behavior, but it's probably the best we can do while staying compliant with the spec.

[donbourne]

I think the line that needs tweaking is "In these cases, where multiple metric instances exist corresponding to the instances of the CDI bean" . From a CDI perspective there might be separate interceptors created but I don't think we ever intended for those separate instances to talk to separate metric instances.

[kenfinnigan]

From discussions I've had with @ebullient and some CDI experts, this really should be a part of Metrics 3.0.

Either marking it deprecated or even better a complete removal.

Having the specification allow fields/methods annotated with @Produces creating CDI beans without any equivalent @Inject is extremely problematic, and as @jmartisk pointed out can lead to ambiguous resolution.

@jmartisk @donbourne, is there any chance we can get this removed from the spec for Metrics 3.0?

[ebullient]

Specifically, deprecate @Produces @Metric ASAP (even in 2.x to discourage use), and remove from 3.0.

[jmartisk]

+1, glad to finally see a strong opinion here. I'll try to come up with a PR for removing this in 3.0, hope it won't have any consequences I haven't foreseen

[Emily-Jiang]

Sorry to be late on this issue! I don't see a big impact on this. Let's take a well-known CDI Bean provided by the JMS:

@Inject
private JMSContext context1;

If an application wants to produce a JMSContext, there are a couple of things can be done and clearly mentioned in the CDI spec.

• Use a qualifier

• Use Alternative or Specialize to ignore the one from the runtime if the end user knows what he/she is doing.

Besides, CDI itself provides CDI bean HttpServletRequest, HttpSession, etc. This is not the valid reason to remove the Metric injection.

[jmartisk]

To be clear, this change does NOT prevent applications from using CDI producers to produce injectable metric objects, nor does it remove the option to @Inject metrics.

If the developer has a strategy to overcome the "problem 1" from the description, they can still use producers. They will just have to register the metric programmatically into the registry if it should appear in the exported data.

Not registering the metrics automatically helps us prevent the issues from "problem 2" where it can lead to funny behavior if the metric is not @ApplicationScoped.

[ebullient]

@Inject is fine.

@Produces is where the problem lies. Programmatically adding the pre-created metric is also a problem. Allowing the user to create their own metric implementation is problematic. There isn’t a good reason to allow a user of the API to define their own implementation of a Counter/Gauge/whatever.

[kenfinnigan]

@mkouba and @antoinesd might want to provide a perspective from CDI.

But as @ebullient said, @Inject isn't the issue. It's allowing @Produces without any @Inject present and the CDI Bean is still created, which is not in the spirit of the CDI specification

[Emily-Jiang]

I read this again. Another solution is to update Metric to be a qulifier and make the name as the binding name. In this case, there will not be umbiguous resolution.

It's allowing @Produces without any @Inject present and the CDI Bean is still created, which is not in the spirit of the CDI specification @Produces is to produce a bean for injection. Are you saying the bean is produced but not being able to be injected? With what I suggested above should solve the problem if I understand this issue correctly.

[kenfinnigan]

The issue isn't just an ambiguous resolution.

It's the fact that a CDI Bean is produced from a @Producer field/method without there being an @Inject anywhere in the user application code.

My understanding is that the CDI specification states that any @Producer method/field that doesn't have a corresponding @Inject present is not called. What MP Metrics is doing here ignores that tenet

[Emily-Jiang]

Thank you @kenfinnigan for your explaination! I now undestood the issue better! One puzzle I have is that how the runtime passsed all of the TCKs to be deleted. How to force @Produces method called? @jmartisk @donbourne ?

[kenfinnigan]

It placed a requirement on any implementation to "fake" an @Inject point and create the CDI Beans anyway

[Emily-Jiang]

Thanks @kenfinnigan ! @ebullient showed me some fake code. I am ok for this issue to be resolved. I just have one more comments for clearly documenting this under Breaking Changes in release note @jmartisk .

[antoinesd]

The right thing to do here is to stop instantiating metrics eagerly from producer detection. Implementations should probably use the ProcessProducer extension event to add specific behavior when producing an instance. This behavior would be to check in the registry before creating the metric. This way the code would be call only when creating an instance for an injection point.

[ebullient]

The right thing to do here is to stop instantiating metrics eagerly from producer detection. Implementations should probably use the ProcessProducer extension event to add specific behavior when producing an instance. This behavior would be to check in the registry before creating the metric. This way the code would be call only when creating an instance for an injection point.

There is no injection point at all, that is the issue. And @Metric is not a Qualifier, so the point is moot.