-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix ObservabilityDevServicesConfigBuildItem usage - can be multiple, fix dup logging output #43945
Conversation
@melloware wdyt? |
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 like it
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview a4b05f7 has been successfully built and deployed to https://quarkus-pr-main-43945-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
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
Map<String, String> runtimeConfig = configProps.get().getConfig(); | ||
String grafanaUrl = runtimeConfig.getOrDefault("grafana.endpoint", ""); | ||
if (configProps != null) { | ||
configProps.forEach(cp -> { |
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.
No need to change it here but we should really avoid using lambdas when they have no value. In this case a for loop would have been better.
Also configProps
should never be null.
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.
Fixed
@gsmet @melloware @brunobat any idea why am I not seeing any of this log from ObservabilityContainer when starting LGTM ...
all I get is this -- which is still fine -- but would also like to see the stuff above |
...ent/src/main/java/io/quarkus/observability/deployment/devui/ObservabilityDevUIProcessor.java
Show resolved
Hide resolved
...containers/src/main/java/io/quarkus/observability/testcontainers/ObservabilityContainer.java
Show resolved
Hide resolved
OK, this is the "culprit" - in ObservabilityDevServiceProcessor ...
|
This comment has been minimized.
This comment has been minimized.
OK, fixed that "missing" log stuff as well. |
Status for workflow
|
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.
It looks good to me now.
Status for workflow
|
There can (and probably will be) multiple ObservabilityDevServicesConfigBuildItem instances,
since there can be multiple DevResources instances.
Currently only the last one would probably be used with this
Optional<ObservabilityDevServicesConfigBuildItem>
parameter?This fixes ObservabilityDevServicesConfigBuildItem usage, so all are checked for potential DevUI contribution.