support per-record observations in batch listeners #3944
Conversation
igormq
force-pushed
the
codex/fix-issue-with-kafka-header-extraction
branch
from
df42ec9 to
351e874
Compare
document recordObservationsInBatch container property describe new option in the change history add integration test for per-record observations Signed-off-by: Igor Macedo Quintanilha <[email protected]>
igormq
force-pushed
the
codex/fix-issue-with-kafka-header-extraction
branch
from
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
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
| } | ||
| throw e; | ||
| } | ||
| finally { |
There was a problem hiding this comment.
if we switch to Observe.observe(), then there is no need for this finally block as observe() will implicitly do that for you.
| if (!isListenerAdapterObservationAware()) { | ||
| // Stop observations | ||
| for (Observation observation : observations) { | ||
| observation.stop(); |
There was a problem hiding this comment.
No need to stop when using observe().
...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
…KafkaMessageListenerContainer.java Co-authored-by: Michal Domagala <[email protected]> Signed-off-by: Igor Macedo Quintanilha <[email protected]>
Don't know if i am doing the right approach here, but.
Related to #3872 (comment)