-
Notifications
You must be signed in to change notification settings - Fork 566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generalize the OCI metrics integration, using qualified injection of Config
#8410
Conversation
...ations/oci/metrics/cdi/src/main/java/io/helidon/integrations/oci/metrics/cdi/OciMetrics.java
Show resolved
Hide resolved
@Produces | ||
@OciMetrics | ||
public Config config(Config config) { | ||
return config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double-checking that you want to return @Default Config
as (also) @OciMetrics Config
. I guess this is the "backstop" implementation, so that users supplying their own @OciMetrics Config
will have their stuff picked up first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I will add a comment explaining that this is intended.
|
||
import io.helidon.config.Config; | ||
|
||
@Priority(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general if you can pick a constant from (bizarrely, counterintuitively, but hey, it's convention) the Interceptor.Priority
(!) constants it's best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
|
||
@Produces | ||
@OciMetrics | ||
public Config config(Config config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be private static
if you wanted; then no instance of OciMetricsConfigProducer
will have to be created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not seem to be able to keep in my head the rules/conventions for visibility to CDI vs. Java declarations. I'll make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair: it's just another method. If you wanted to surface it, leave it as public
for sure. But often producer methods are to be used solely by the container. If that's the case as I strongly guess it is, then I personally prefer to limit visibility wherever I can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some comments
Closing - this PR is obsolete in favor of superseding PR #8419 |
Description
Configuration for the OCI metrics integration might come from different sources. This PR slightly generalizes how the
Config
is injected into the bean which starts up the integration.The "start-up" bean now injects a
@Qualified
Config
instance and provides a default producer for that qualifiedConfig
which is the normally-injectedConfig
itself. There is no change in the behavior of this library when used as in the past.The change is if another library includes a higher-priority bean which also produces the qualified
Config
bean, in which case this library uses that config instance instead.Documentation
No functional change, and we do not intend for users to take advantage of this generalization themselves.
No doc impact.