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

Adding custom ObservationHandler does not preserve the default ObservationHandlers #34510

Open
Tracked by #35776
violetagg opened this issue Mar 8, 2023 · 11 comments
Open
Tracked by #35776
Labels
status: blocked An issue that's blocked on an external project change theme: observability Issues related to observability type: enhancement A general enhancement
Milestone

Comments

@violetagg
Copy link
Member

violetagg commented Mar 8, 2023

Spring Boot version

3.0.4

Reproducible example

https://github.com/violetagg/netty-observation-repro

Problem description

I would like to use Reactor Netty tracing capabilities in Spring Boot application.
For this purpose I need to reuse the ObservationRegistry created by Spring Boot and to add ObservationHandlers provided by Reactor Netty.

Observations:

  1. If I only reuse the ObservationRegistry without adding the ObservationHandlers provided by Reactor Netty - the tracing information is correctly visualised but the span tags are duplicated and one and the same Timer object is created every time.

Screenshot 2023-03-08 at 9 42 13 Screenshot 2023-03-08 at 9 42 25

  1. If I reuse the ObservationRegistry and add ObservationHandlers provided by Reactor Netty - the tracing information is NOT correctly visualised.

  2. If I reuse the ObservationRegistry, add ObservationHandlers provided by Reactor Netty and in addition add all default ObservationHandlers provided by Spring Boot - the tracing information is correctly visualised, the span tags are not duplicated and one and the same Timer object is created only once. Reactor Netty tracing capabilities are working as expected.

Screenshot 2023-03-08 at 9 51 18 Screenshot 2023-03-08 at 9 51 29

Desired solution

Spring Boot should preserve the default ObservationHandlers when custom ObservationHandlers are added to the configuration.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 8, 2023
@violetagg
Copy link
Member Author

/cc @tmarback @marcingrzejszczak

@wilkinsona
Copy link
Member

See also #34399.

@violetagg
Copy link
Member Author

yep, because of the mentioned issue, in the example I added @Order when declaring the various ObservationHandlers

@marcingrzejszczak
Copy link
Contributor

marcingrzejszczak commented Mar 8, 2023

I think that the default observation handlers should not have conditional on missing bean

@wilkinsona
Copy link
Member

That doesn't feel very Boot-like to me. It would leave users who want to have the auto-configured handlers back off and replaced with their own with a problem. They'd have to exclude ObservationAutoConfiguration which would lose more than just the default observation handlers.

@marcingrzejszczak
Copy link
Contributor

If you don't want the default tracing handler you register your own tracing handler before the default one because only one can be used. Same for metrics. We are grouping all handlers for exactly this reason - that you can put your own handlers before the existing ones.

Another option for @violetagg would be never to extend from the ones that we provide but delegate to them (that way the types would not be the same and @ConditionalOnMissingBean wouldn't have effect).

@violetagg
Copy link
Member Author

If you don't want the default tracing handler you register your own tracing handler before the default one because only one can be used. Same for metrics. We are grouping all handlers for exactly this reason - that you can put your own handlers before the existing ones.

Yes we do exactly that our tracing handlers are implemented in a way that io.micrometer.tracing.handler.TracingObservationHandler#supportsContext handles only our Reactor Netty implementation, everything else should be handled by the default configuration in Spring Boot.

The scenario here is that we do want the default Spring Boot tracing for the server. What we want to enable is the tracing for Reactor Netty HttpClient only.

Another option for @violetagg would be never to extend from the ones that we provide but delegate to them (that way the types would not be the same and @ConditionalOnMissingBean wouldn't have effect).

@marcingrzejszczak
Copy link
Contributor

The scenario here is that we do want the default Spring Boot tracing for the server. What we want to enable is the tracing for Reactor Netty HttpClient only.

Yeah so all of your custom handlers wouldn't extend from the default ones, they would delegate to them. So you would never hit the @ConditionalOnMissingBean problem

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Mar 15, 2023
@violetagg
Copy link
Member Author

The scenario here is that we do want the default Spring Boot tracing for the server. What we want to enable is the tracing for Reactor Netty HttpClient only.

Yeah so all of your custom handlers wouldn't extend from the default ones, they would delegate to them. So you would never hit the @ConditionalOnMissingBean problem

So we have to delegate to the default in both cases when in Spring Boot application and when in standalone Reactor Netty.
For Spring Boot we will delegate to the handlers created by Spring Boot.
In the standalone case to what should we delegate?

@marcingrzejszczak
Copy link
Contributor

So if you have 3 custom types that extend default, sending and receiving then you should in all three cases not extend but delegate to these 3. That way in Boot you will have 6 handlers registered but yours should be first (in order - your sending or receiving then your default and then the boot ones). In the case of standalone you need to do the same:

new FirstMatchingComposite...(

  • your sending
  • your receiving
  • your default
  • micrometer sending
  • micrometer receiving
  • micrometer default
    )

@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Mar 15, 2023
@violetagg
Copy link
Member Author

violetagg commented Mar 23, 2023

@marcingrzejszczak and I tried the variant with delegation. (reactor/reactor-netty@e02e722)
Although technically this is possible it requires 3 TracingObservationHandlers which do not have functional logic but just delegate and this should be a way of implementing custom TracingObservationHandler and registering it as a Bean for every library not just Reactor Netty.

On the other hand if default TracingObservationHandlers are not conditional and we rely on the io.micrometer.tracing.handler.TracingObservationHandler#supportsContext then if the custom TracingObservationHandler supports this context it will be the one that will be chosen otherwise the default TracingObservationHandlers. (as FirstMatchingCompositeObservationHandler is used when configuring the ObservationRegistry)

Basically the variant with delegation is a workaround to have the default TracingObservationHandlers in place.

@philwebb philwebb added status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 7, 2023
@philwebb philwebb added this to the 3.x milestone Jun 7, 2023
@philwebb philwebb mentioned this issue Jun 7, 2023
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked An issue that's blocked on an external project change theme: observability Issues related to observability type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants