-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Enhancement] Tries to inject Vertx instance from CDI using a qualifier #381
Conversation
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.
@gaol, This looks good. Once the formatting checks have been satisfied, I can merge this.
@jasondlee from the discussion in smallrye/smallrye-reactive-messaging#2772, do you see problem if using Identifier instead of Named qualifier in smallrye-opentelemetry ? which would mean that there will be a dependency of |
@jasondlee I updated the PR to use |
...n/java/io/smallrye/opentelemetry/implementation/exporters/AbstractVertxExporterProvider.java
Outdated
Show resolved
Hide resolved
Hello @jasondlee , I updated according to your feedback to log a warning message and return a new Vertx instance in case it cannot resolve from CDI. Would you please review again ? thank |
...n/java/io/smallrye/opentelemetry/implementation/exporters/AbstractVertxExporterProvider.java
Outdated
Show resolved
Hide resolved
...n/java/io/smallrye/opentelemetry/implementation/exporters/AbstractVertxExporterProvider.java
Outdated
Show resolved
Hide resolved
...n/java/io/smallrye/opentelemetry/implementation/exporters/AbstractVertxExporterProvider.java
Show resolved
Hide resolved
Just interjected a few comments, feel free to ignore or do whatever you wish with them :-) |
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 left one last comment.
...n/java/io/smallrye/opentelemetry/implementation/exporters/AbstractVertxExporterProvider.java
Show resolved
Hide resolved
...n/java/io/smallrye/opentelemetry/implementation/exporters/AbstractVertxExporterProvider.java
Show resolved
Hide resolved
@jasondlee I think exposing one instance of a bean using a qualifier can also leave it to the lookup side to decide if it wants the bean or not. Trying to lookup the bean without qualifier will hand over the decision out :) I remembered in the zulip discussion that we want the lookup side be able to opt in, at least in the start time of integrating the vertx subsystem. Please feel free to correct me if I am wrong. :) |
I just reread that and you're correct. Given that, I think this looks great. I'll get it merged. Thanks! |
HI, @gaol. One more thing: commits to this repo have to be signed. Once you've signed this commit, I'll merge it. |
@jasondlee Updated to signed commit :) |
It's still complaining that the commit isn't signed. Have you done this? https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
@jasondlee sorry, I got the wrong idea, I understood that as |
…ng a qualifier Signed-off-by: Lin Gao <aoingl@gmail.com>
@jasondlee finally, I got the the signed commit :) |
Fixes #380
This PR tries to add a new config
otel.exporter.vertx.cdi.identifier
to tries to inject the Vertx instance from the CDI with the qualifier, and it falls back to current way ofVertx.vertx()
if there is no such configuration to keep compatible.