-
Notifications
You must be signed in to change notification settings - Fork 615
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
GH-2650: Observability enhancements in reactive Kafka binder #3003
GH-2650: Observability enhancements in reactive Kafka binder #3003
Conversation
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.
Otherwise it is all good.
From my point of view.
I'd be glad to hear some other opinion to make the solution as easy as possible.
Thanks
@@ -199,7 +199,7 @@ Function<Flux<ReceiverRecord<byte[], byte[]>>, Flux<String>> lowercase() { | |||
@Bean | |||
java.util.function.Consumer<Flux<String>> patternConsumer() { | |||
return f -> f.doOnNext(s -> patternedDeliveries.add(s)) | |||
.subscribe(); | |||
.subscribe(); |
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.
Does not look like the change in this class is meaningful.
Might be just fully reverted since PR is already complex enough.
/** | ||
* @author Artem Bilan | ||
* @author Soby Chacko | ||
* @since 4.1.1 |
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 think we need to revise the version we are introduce this feature in.
}) | ||
@DirtiesContext | ||
@AutoConfigureObservability | ||
@EmbeddedKafka(topics = { "foobar" }) |
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.
You know what is my attitude for this kind of words...
</root> | ||
<root level="WARN"> | ||
<appender-ref ref="stdout"/> | ||
</root> |
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 think we can defer this file refactoring to some other commit: the PR is already too complex to have this extra noise as well.
@@ -139,6 +142,10 @@ | |||
@ConditionalOnBean(FunctionRegistry.class) | |||
public class FunctionConfiguration { | |||
|
|||
private static final boolean isContextPropagationPresent = ClassUtils.isPresent( |
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.
Right. That's what I meant to have context-propagation
dep as optional
.
|
||
In this section, we will describe how Micrometer based observability is enabled in the reactive Kafka binder. | ||
|
||
There is built in support for observability when it comes to producer binding, but you need to opt-in for this by enabling the following property. |
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 thought built-in
comes as a one word.
But I won't mind since I'm still learning English 😄
|
||
When this property is set to `true`, you can trace the publishing of records. | ||
|
||
Both publishing records using `StreamBridge` and regular `Supplier<?>` beans can be now traced when enabling the above property. |
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 guess better to say observed
since an Observation is not only for tracing.
|
||
However, on the consumer side, enabling observability is not as straightforward as on the producer side. | ||
|
||
There are two starting points for consumer binding - one a topic where the data is published via a producer binding, another one where the data is produced via not Spring Cloud Stream. |
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.
not via
?
I won't mind otherwise, just don't sounds for me.
Again: English is not my native language 😄
Fixes spring-cloud#2650 * Enable native observability support for output binding in the reactive Kafka binder * Adding test to verify this support with downstream consumers * Adding ref docs
4ae53f3
to
b8e6102
Compare
Fixes #2650