-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Allow configuring observation registry directly #3643
Allow configuring observation registry directly #3643
Conversation
@ cfredri4 I think this needs more discussion. These changes modify the behavior of setting the default |
This was for simplicity's sake as the registry is nullable and null and NOOP are treated the same. But I can change back to NOOP if desired.
I have two situations where this change would be beneficial:
Both of these situations would be trivial had it been possible to configure the observation registry directly. |
@cfredri4 We are ok with the changes. However, could you re-work your solution so we don't break any existing behavior? Specifically, the default |
Also, please add the author tag with your name in all the classes you changed. Thanks! |
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.
Please, consider to address reviews by tomorrow.
We have release on Monday, so might be great to have this in.
Thank you!
spring-kafka/src/main/java/org/springframework/kafka/core/KafkaTemplate.java
Outdated
Show resolved
Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/core/KafkaTemplate.java
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Show resolved
Hide resolved
c378bc1
to
b8027b7
Compare
I have pushed some updates, please check. |
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
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.
Well, sorry for the noise, but apparently there are some Checkstyle violations:
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/core/KafkaTemplate.java:496: Line matches the illegal pattern 'Trailing whitespace'. [Regexp]
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/listener/ContainerProperties.java:45:1: Wrong order for 'io.micrometer.observation.ObservationRegistry' import. [ImportOrder]
> Task :spring-kafka:checkstyleMain
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java:81:8: Unused import - org.springframework.beans.factory.ObjectProvider. [UnusedImports]
Please, consider to use Gradle check
task before pushing commits to the PR.
Thanks
This changes allows configuring the observation registry directly, instead of it being fetched from the application context. This is to allow observability when KafkaTemplate/KafkaMessageListenerContainer are used without an application context.
b8027b7
to
16f251c
Compare
Fixed. |
This changes allows configuring the observation registry directly, instead of it being fetched from the application context. This is to allow observability when KafkaTemplate/KafkaMessageListenerContainer are used without an application context.