-
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? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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
batch, similar to a record listener. | ||
This allows you to have visibility into the processing of each record, even within a | ||
batch context. | ||
No spans will be created for the batch itself, only for each record. |
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'm not sure if you broke something with your change, but we have to revise that an observation is really created for the whole batch around that listener method call.
If that was not supported at all, then it is OK.
spring-kafka-docs/src/main/antora/modules/ROOT/pages/whats-new.adoc
Outdated
Show resolved
Hide resolved
Signed-off-by: Igor Macedo Quintanilha <igor.quintanilha@teya.com>
a50c3c9
to
a4251a9
Compare
Don't know if i am doing the right approach here, but.
Related to #3872 (comment)