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

Support for Micrometer Tracing Annotations #36001

Closed
Exidex opened this issue Jun 20, 2023 · 6 comments
Closed

Support for Micrometer Tracing Annotations #36001

Exidex opened this issue Jun 20, 2023 · 6 comments
Labels
for: external-project For an external project and not something we can fix status: superseded An issue that has been superseded by another theme: observability Issues related to observability

Comments

@Exidex
Copy link

Exidex commented Jun 20, 2023

In version 1.1 Micrometer Tracing ported annotations from Sleuth. To ease the migration from the Spring Boot 2 are there any plans to add support for these annotations?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 20, 2023
@jonatan-ivanov
Copy link
Member

Have you tried to create a SpanAspect @Bean?
Right now annotation support for Micrometer and Micrometer Tracing is not auto-configured, you need to create XyzAspect beans.

@jonatan-ivanov jonatan-ivanov added status: waiting-for-feedback We need additional information before we can continue theme: observability Issues related to observability and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 20, 2023
@Exidex
Copy link
Author

Exidex commented Jun 20, 2023

I have but I haven't managed to get it working. But even if I did the documentation for AOP configuration mentions that it will not work in some cases where Sleuth worked. Additionally you need to do some boilerplate bean creation which is usually done by Spring Boot. Given that Micrometer Tracing is advertised as full flagged replacement for Sleuth I would have expected it to work in the same cases but maybe that is too much to ask. So far migration has been very problematic because of this.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 20, 2023
@jonatan-ivanov
Copy link
Member

Could you please provide a minimal sample project to reproduce this?

... the documentation for AOP configuration mentions that it will not work in some cases where Sleuth worked. Additionally you need to do some boilerplate bean creation ...

Could you please tell us more what do you mean by this? Sleuth used the same AOP mechanism (Spring AOP + AspectJ).

Given that Micrometer Tracing is advertised as full flagged replacement for Sleuth

Where did you hear/read this? We might need to refine docs/talks: Micrometer Tracing is pretty similar to Sleuth (we copied Sleuth and modified things) but Micrometer Tracing does not have Spring dependencies (so no instrumentation in it).

So far migration has been very problematic because of this.

I'm sorry to hear this, we are definitely interested in feedback about migration pain.

@jonatan-ivanov jonatan-ivanov added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 20, 2023
@Exidex
Copy link
Author

Exidex commented Jun 21, 2023

Could you please tell us more what do you mean by this?

Here is PR which explicitly mentions that these annotations will not work on the interfaces

feature parity will not be full cause we don't have access to Spring code in Micrometer so aspects will not work as well as they did in Sleuth. The difference will be that annotations (@NewSpan and @ContinueSpan) on interfaces will not be supported.

Other issue I encountered is that SpanAspect will throw NoSuchMethodException on non public methods. Both of those can be worked around but it is pretty annoying that it doesn't work when it previously did without mention in migration guide

Where did you hear/read this?

spring-cloud-sleuth and spring-cloud-sleuth-otel both link to this migration guide which reads:

The whole Spring Cloud Sleuth API module got pretty much moved to Micrometer Tracing. That means that in the vast majority of places you should just change the package from org.springframework.cloud.sleuth to io.micrometer.tracing.

observability-with-spring-boot-3 goes as far as saying:

Micrometer Tracing (formerly Spring Cloud Sleuth).

So my assumption was that after changing to io.micrometer.tracing.annotation annotations would work because all the needed instrumentation would be in the spring boot which tuned out to be not the case. And only after diving into Micrometer Tracing documentation I could get it to work. If we started update to Spring Boot 3 earlier the instrumentation for the annotation was not even present in Micrometer Tracing until version 1.1.

In general it feels like the migration to Micrometer Tracing was pretty rushed as there's a good chunk missing functionality in Spring Boot 3 even half a year later. There's even issue to fill the holes.

So my only request would be to fill the hole of missing autoconfiguration for Micrometer Tracing annotations.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 21, 2023
@jonatan-ivanov
Copy link
Member

Here is micrometer-metrics/tracing#185 which explicitly mentions that these annotations will not work on the interfaces

It also mentions why, could you please open an issue in Micrometer Tracing for this?

Other issue I encountered is that SpanAspect will throw NoSuchMethodException on non public methods. Both of those can be worked around but it is pretty annoying that it doesn't work when it previously did without mention in migration guide

This might be because of cglib vs. JDK proxy, I think I would open a separate issue in Micrometer Tracing to start.

spring-cloud-sleuth and spring-cloud-sleuth-otel both link to this migration guide which reads: ...

I don't see it saying/advertising that it would be a "full flagged replacement for Sleuth". What I see is: "got pretty much moved" and " vast majority of places ...", to me this means that most of the things work the same but there are differences which kind of reflects the reality. please let us know if you have ideas for improving the docs, PRs are always welcomed.

So my assumption was that after changing to io.micrometer.tracing.annotation annotations would work because all the needed instrumentation would be in the spring boot which tuned out to be not the case.

I'm sorry this is not what you expected, we are doing or best to make migration easy, though there always will be differences. Some of them are intentional and some of them might be because we made a mistake, pull requests and issues are always welcome. Also to be fair, you are not just doing a major release upgrade but you are moving from one project to another, I think it is a fair assumption to make that maybe there will be things that are different.

If we started update to Spring Boot 3 earlier the instrumentation for the annotation was not even present in Micrometer Tracing until version 1.1.

We are suggesting most of the users to use the @Observed annotation instead and we did not get much complaints about the missing @NewSpan. I recommend to do that to you too.

In general it feels like the migration to Micrometer Tracing was pretty rushed as there's a good chunk missing functionality in Spring Boot 3 even half a year later. There's even #35776 to fill the holes.

So my only request would be to fill the hole of missing autoconfiguration for Micrometer Tracing annotations.

If you look into those issues there are workarounds for most of them so users can resolve them by creating beans or setting properties. Which is definitely not ideal but not catastrophic either (that's why we are fixing them). Please let us know if you want to help us out and what to open a PR to resolve them.

Since it seems this is at least 3 issues together and two of them seems to be in the Micrometer Tracing repo, let me close this and add the SpanAspect piece to #35191. Could you please open the two other issues you had in Micrometer Tracing?

@jonatan-ivanov jonatan-ivanov closed this as not planned Won't fix, can't repro, duplicate, stale Jun 23, 2023
@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jun 23, 2023

TL;DR: should be partly superseded by #35191 and partly belong to Micrometer Tracing.

@jonatan-ivanov jonatan-ivanov added for: external-project For an external project and not something we can fix status: superseded An issue that has been superseded by another and removed status: feedback-provided Feedback has been provided labels Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: superseded An issue that has been superseded by another theme: observability Issues related to observability
Projects
None yet
Development

No branches or pull requests

3 participants