-
Notifications
You must be signed in to change notification settings - Fork 1.6k
support per-record observations in batch listeners #3944
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
base: main
Are you sure you want to change the base?
support per-record observations in batch listeners #3944
Conversation
351e874
to
46a21b6
Compare
@igormq Thank you for the PR. We will do a detailed review soon. But, please add your name as an author to the classes you modified. Also, make the modifications in the ref docs directly and then add a short sentence about it in the |
spring-kafka/src/main/java/org/springframework/kafka/listener/ContainerProperties.java
Outdated
Show resolved
Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/ContainerProperties.java
Outdated
Show resolved
Hide resolved
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.
Thank you for looking into this!
Not easy task to tackle.
Hope you'll find my review as reasonable.
spring-kafka-docs/src/main/antora/modules/ROOT/pages/appendix/change-history.adoc
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
@igormq Thank you for this PR. Implementing observability with batch listeners is not trivial, and I think you laid good foundations here. Providing this as an optional feature is very important, as we don't want to impose observability by default in all batch listeners. Please take a look at some of the feedback I provided, which largely echoes what @artembilan has already reviewed. As you make further changes, we will need to add docs and other related components, but getting the implementation right is the critical step right now. Thanks again! |
Hey @sobychacko and @artembilan thank you so much for all the feedback, really appreciate! I think that i addressed all of them. Let me know if I am missing something :) |
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
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.
@igormq Code changes look good. Now, we need to add reference docs with instructions on enabling observability in batch mode, maybe some examples etc. Then we need to mention this new feature in the whats-new
doc.
7f45017
to
e32b309
Compare
done that, and also fixed the check style issues |
spring-kafka-docs/src/main/antora/modules/ROOT/pages/micrometer.adoc
Outdated
Show resolved
Hide resolved
spring-kafka-docs/src/main/antora/modules/ROOT/pages/micrometer.adoc
Outdated
Show resolved
Hide resolved
spring-kafka-docs/src/main/antora/modules/ROOT/pages/micrometer.adoc
Outdated
Show resolved
Hide resolved
spring-kafka-docs/src/main/antora/modules/ROOT/pages/micrometer.adoc
Outdated
Show resolved
Hide resolved
spring-kafka-docs/src/main/antora/modules/ROOT/pages/micrometer.adoc
Outdated
Show resolved
Hide resolved
spring-kafka-docs/src/main/antora/modules/ROOT/pages/whats-new.adoc
Outdated
Show resolved
Hide resolved
Signed-off-by: Igor Macedo Quintanilha <[email protected]>
a50c3c9
to
a4251a9
Compare
spring-kafka-docs/src/main/antora/modules/ROOT/pages/whats-new.adoc
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...java/org/springframework/kafka/support/micrometer/BatchIndividualRecordObservationTests.java
Outdated
Show resolved
Hide resolved
69c4cb5
to
60177f8
Compare
Signed-off-by: Igor Macedo Quintanilha <[email protected]>
60177f8
to
94d1ee6
Compare
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.
Just one concern for docs looking into this from newcomer perspective.
Thanks
} | ||
---- | ||
|
||
When this property is `true`, an observation will be created for each record in the batch, but the observation is not propagated to the listener method. |
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.
OK. It is not propagated to the listener, but can we have an explanation what happens instead then?
Don't know if i am doing the right approach here, but.
Related to #3872 (comment)