Skip to content
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

Customize the Observation created by MicrometerObservationListener #3455

Closed
nvervelle opened this issue May 9, 2023 · 10 comments
Closed

Customize the Observation created by MicrometerObservationListener #3455

nvervelle opened this issue May 9, 2023 · 10 comments
Labels
type/enhancement A general enhancement
Milestone

Comments

@nvervelle
Copy link
Contributor

nvervelle commented May 9, 2023

Motivation

The current integration between reactor and micrometer, with tap() and Micrometer.observation(), only allows simple use cases for the creation of the Observation that will observe the reactive pipeline : it's currently not possible to provide a context supplier or a convention that will be used to create the Observation.

One example where the current solution is limiting is when using Kafka in spring boot where the listener is a Flux, each kafka message becoming an element of the Flux. It's not possible to extract the traceparent header from the kafka message to use it when creating the Observation.

Desired solution

I'd like to be able to give more control on what Micrometer.observation() is doing. My idea is to be able to provide an optional Function<ObservationRegistry, Observation> so that the Observation will be created with as much customization as required.

Considered alternatives

I first tried a solution that will not imply any modification on reactor-micrometer library : I had to copy/paste 3 package private classes from the library (MicrometerObservationListener, MicrometerObservationListenerConfiguration and MicrometerObservationListenerFactory) to be able to modify them. It's not a good solution in the long term, as it needs to keep the 3 modified classes in sync with the library.

Additional context

I have submitted a PR with my ideas about the Function<>: #3456

@reactorbot reactorbot added the ❓need-triage This issue needs triage, hasn't been looked at by a team member yet label May 9, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue May 9, 2023
@OlegDokuka
Copy link
Contributor

OlegDokuka commented May 19, 2023

Hi, @nvervelle!

Thanks for good idea. Can you please provide a code sample of how that could be used if you have such API.

Best,
Oleh

@OlegDokuka OlegDokuka added the status/need-user-input This needs user input to proceed label May 19, 2023
@nvervelle
Copy link
Contributor Author

Hi @OlegDokuka,

Yes, sure. It would be useful for example for observing kafka listeners in Spring Boot. A very simplified example compared to what I currently have to do to observe single messages in the receiver flux. Tell me if it's enough.

import io.micrometer.observation.Observation;
import org.springframework.kafka.support.micrometer.KafkaListenerObservation;
import org.springframework.kafka.support.micrometer.KafkaRecordReceiverContext;
import reactor.core.observability.micrometer.MicrometerObservationListenerFactory;
import reactor.kafka.receiver.KafkaReceiver;
import reactor.kafka.receiver.ReceiverRecord;

KafkaReceiver.create(buildReceiverOptions(...))
    .receive()
    .flatMap(this::receiveRecord);

private Mono<Boolean> receiveRecord(ReceiverRecord receiverRecord) {
    return Mono.defer(() -> Mono.defer(...).contextCapture())
        .name("receiveRecord")
        .tap(MicrometerObservationListenerFactory.observation(
            observationRegistry,
            // HERE: I can customize the observation created by tap() with both
            // * a convention specialized for kafka messages
            // * a context supplier that will retrieve context information from the kafka message header
            registry -> Observation.createNotStarted(
                KafkaListenerObservation.DefaultKafkaListenerObservationConvention.INSTANCE,
                () -> new KafkaRecordReceiverContext(receiverRecord, "listenerId", () -> "clusterId"),
                registry)));
}

@OlegDokuka
Copy link
Contributor

Makes sense to me now. Do you mind signing CLA so i can merge your PR?

@nvervelle
Copy link
Contributor Author

I asked my legal department to sign the CLA or give me permission to sign it, I hope I will have their approval in the next few days...

@nvervelle
Copy link
Contributor Author

Hi @OlegDokuka, I've signed the CLA, so you can merge the PR if you want 👍

@nvervelle
Copy link
Contributor Author

Hi @OlegDokuka , any hope to merge the PR in the near future ?

@chemicL
Copy link
Member

chemicL commented Jun 20, 2023

@nvervelle I asked for some tests and documentation in the PR, hopefully we can ship it in the next release :)

nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jun 26, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jun 26, 2023
@nvervelle
Copy link
Contributor Author

@chemicL I've pushed a commit for the tests and documentation

nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jun 27, 2023
@ajax-osadchuk-r
Copy link

This improvement will help my team a lot, we are also looking for a way to properly use the micrometer with the reactor-kafka.
@nvervelle your solution seems to actually what we need.
@chemicL Looking forward to get this feature released.

nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jun 29, 2023
@OlegDokuka OlegDokuka added type/enhancement A general enhancement and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet status/need-user-input This needs user input to proceed labels Jun 30, 2023
@OlegDokuka OlegDokuka modified the milestone: 3.5.8 Jun 30, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jul 4, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jul 5, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jul 5, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jul 5, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jul 5, 2023
nvervelle added a commit to nvervelle/reactor-core that referenced this issue Jul 5, 2023
@chemicL chemicL closed this as completed in 7d27788 Jul 5, 2023
@chemicL
Copy link
Member

chemicL commented Jul 5, 2023

@nvervelle thank you for the contribution! This change will be part of 3.5.8 and 3.6.0-M1 🚀 Looking forward to more contributions 👏

@chemicL chemicL added this to the 3.5.8 milestone Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants