eclipse / microprofile-metrics

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

Update RegistryScope to be qualifer #778

Closed Channyboy closed 1 year ago

Channyboy commented 1 year ago

fixes #749

Channyboy commented 1 year ago

I believe you were referring to the @Default used for a CDI Producer to handle any injections where no matching producer with a specific @RegistryScope's scope attribute is present. This would be the case since the scopes would not be known ahead of time.

The @Default javadoc says:

If a bean does not explicitly declare a qualifier other than @Named, the bean has the qualifier @Default.

This would only apply to any injections for a MetricRegistry without any qualifiers. This would apply to injections that only has @Inject being used and currently with the v5.0 release it would also apply for anything annotated with @RegistryScope since in 5.0 it is not a qualifier.

If we change @RegistryScope to a qualifier and specify the scope attribute to be binding, any injections using @RegistryScope will not apply to the @Default annotated CDI producer for MetricRegistry. A Producer with only @RegistryScope and not defining the scope is not applicable either.

It would throw:

WELD-001475: The following beans match by type, but none have matching qualifiers:

A binding attribute on the qualifier would require a corresponding producer with that qualifier and attribute.

fyi @donbourne

Channyboy commented 1 year ago

@tjquinno Not sure if you saw the update above^^

tjquinno commented 1 year ago

I did see the additional notes but have been slow in responding.

I agree with Daniel's comments. I did some quick experiments, probably similar to the ones Daniel did, and found the same results. Chalk my earlier suggestion up to a hazy recollection of exactly how producers and qualifiers and binding attributes work.

One other minor note...

Even though the example code in the document is just a suggestion, we could condense it a bit if we wanted to by combining the qualified producer and the default producer something like this:

    @Produces
    @Default
    @RegistryScope
    public MetricRegistry getMetricRegistry(InjectionPoint ip) {
        RegistryScope registryScope = ip.getAnnotated()
                .getAnnotation(RegistryScope.class);
        return getOrCreate(registryScope == null ?
                 MetricRegistry.APPLICATION_SCOPE 
                : registryScope.scope());
    }

That requires only a single very short producer method (which handles both cases, whether the injection point is qualified with @RegistryScope or not) at the (small) cost of searching unnecessarily for the RequestScope annotation in cases when we know it won't be there--when the injection point did not have the RequestScope qualifier.

Just a thought.

Channyboy commented 1 year ago

@tjquinno I think @Default can only be used by itself and can not be combined with another qualifier.

I went ahead and tried it out and using the @RegistryScope with @Default would produce a similar WELD error where it cannot find the right producer.

WELD-001475: The following beans match by type, but none have matching qualifiers:
tjquinno commented 1 year ago

Here is an example testbed I created that, I think, shows the single-producer approach working. It's possible I've missed something that incorrectly makes this example seems to work.

Note that the example does not use @RegistryScope itself but rather a custom qualifier, bean, and producer. Maybe that's part of the difference between what we're seeing.

https://github.com/tjquinno/greet-with-producer

It's a simple Helidon project created using our command-line starter, enhanced with a few files to test this scenario. If you build and run the project, then access http://localhost:8080/greet the output shows values from four injected fields, some with the qualifier and some without. The GreetResource class is where the injections occur and MyQualifier, MyBean, and MyProducer are the other interesting classes.

Channyboy commented 1 year ago

@tjquinno Sorry, that was my mistake. I think my MP Metrics dependency wasn't updated properly to the snapshot built off of this branch. It works now ;) I've updated the appendix example in the latest commit.