Add auto-configuration for ObservedAspect

[vpavic]

This adds support for auto-configuring ObservedAspect when AspectJ is on the classpath, which enables the usage of @Observed.

---

Is it possible this gets considered for 3.1? I know the RC1 went out last week, but this is a change of small impact that IMO offers nice convenience for the users.

/cc @jonatan-ivanov

[wilkinsona]

Thanks, @vpavic.

I don't think we should rush into this. There's some similarity with TimedAspect that we do not auto-configure. I have a recollection that there were good reasons for that but a search of the issue tracker hasn't found much to refresh my memory. It may have been related to a possible clash between the aspect and WebMvcMetricsFilter that also looked for @Timed.

[jonatan-ivanov]

Thank you for the PR!

I have mixed feelings about this: On one hand, it would be great if this could work (also @Timed and @Counted) out of the box. On the other hand (these might not be as big of a concerns), I guess this has a small overhead and maybe users does not want to configure the aspects from Micrometer but they want to react to the annotations differently (in that case they can create their own annotations).

To Andy's point: @Timed has a conflict in Boot 2.x because of the filters and AutoTimer: if you added @Timed on a controller method and you also had an aspect configured, you ended up with two timers. This is not the case with 3.x, I don't know though what issues one can get into if we do this (maybe we should also introduce an enabled flag if we want to do this).

[vpavic]

The motivation behind this PR is that, from my experience, in order to have application-wide observability based on Micrometer the use of @Observed is basically a given for all but the most trivial applications. Situation will improve as Framework adds observability support for its features that can be used to build endpoints of some kind (or more generally, entry points into some processing flow) but real-world applications will still have some use cases for @Observed.

I've never used @Timed and @Counted so I can't speak about the concerns you expressed, but to me it feels like those are a bit different from observability which is a major theme in Spring's recent releases and quite honestly it's a lot of boilerplate work that needs to be repeated from project to project.

[jonatan-ivanov]

I agree on real-world applications will still have some use cases for @Observed even if we would have instrumentation for everything, there is still business logic.

[jonatan-ivanov]

In case we decide to move forward with this, can we also cover TimedAspect, CountedAspect and SpanAspect here? See: https://github.com/spring-projects/spring-boot/issues/36001

[vpavic]

@jonatan-ivanov It's unclear to me who is your question aimed at - is it (someone from) the Spring Boot team, or are you asking me to update the PR and add registration of beans you mentioned?

[jonatan-ivanov]

@vpavic The question is aimed at everyone who is involved in this PR (or will be). You are definitely included, would you be up to adding the other aspects if we decide to go ahead with these?

[vpavic]

If the team decides so, and that helps move things forward, no worries about updating the PR.

[jonatan-ivanov]

Hey @vpavic, We discussed this today and since it is not a risky change and would be a nice addition, we would like to include it in 3.2.0-M1 if possible. Would you be up to updating the PR and add support for TimedAspect, CountedAspect and SpanAspect too?

[vpavic]

Sure, I'll update over the next few days.

[vpavic]

I looked into updating this PR today, but auto-configuring additional aspects isn't as straightforward as ObservedAspect. I'm also not sure whether those should really be a part of the same commit/PR as they come from different Micrometer modules:

• ObservedAspect is a part of micrometer-observation
• SpanAspect is a part of micrometer-tracing
• CountedAspect and TimedAspect are parts of micrometer-core

Therefore they seem to be targeting different auto-configurations:

• ObservedAspect fits into ObservationAutoConfiguration (as proposed by this PR)
• SpanAspect looks like it should fit into MicrometerTracingAutoConfiguration, but according to Micrometer docs the aspect depends on several other components and I'm not sure what's really needed to be auto-configured there - additionally (and interestingly) the documentation describes this aspect as incubating
• I'm not sure where CountedAspect and TimedAspect should be auto-configured, but it doesn't look like they fit into ObservationAutoConfiguration or MicrometerTracingAutoConfiguration

[jonatan-ivanov]

I think CountedAspect and TimedAspect should be "next to" where we auto-configure the MeterRegistry, I would have put it into MetricsAutoConfiguration but it seems it is configuring all the things we need before the registry so I guess it would make sense creating a new class for this, e.g.: MetricsAspectsAutoConfiguration. What do you think?

Regarding SpanAspect, I just looked more into it and I just realized that our ValueResolver implementation always returns null which might not be the best for auto-configuration. I think the "incubating" label can be resolved by Micrometer Tracing 1.2.0 but let me ask Marcin: @marcingrzejszczak Do you want to make SpanAspect stable in Micrometer Tracing 1.2.0 (right now docs says incubating)? If so and we should add auto-configuration for it, should we add a default ValueResolver that does String.valueOf instead of auto-configuring NoOpValueResolver?

[vpavic]

As mentioned in https://github.com/spring-projects/spring-boot/pull/35191#issuecomment-1527906036, I don't have experience with @Timed and @Counted so I really don't have an opinion on how to structure auto-configuration for the related aspects.

Judging by your comment, there seem to be even more question marks around SpanAspect.

This PR was opened as purely focused on observability, and I thought the team acknowledged that by making it a part of observability topic (https://github.com/spring-projects/spring-boot/issues/35776), but having looked into configuring those additional aspects it seems to me that the focus is now being shifted to covering all Micrometer-provided aspects (which is a broader concern than just observability).

Is there anything wrong with proceeding with this PR as-is, and opening separate issues for CountedAspect/TimedAspect and SpanAspect (that one even appears to be blocked), potentially giving an opportunity to someone else from the community to contribute those?

[marcingrzejszczak]

@marcingrzejszczak Do you want to make SpanAspect stable in Micrometer Tracing 1.2.0 (right now docs says incubating)?

I think it's a bug in the docs. This isn't an incubating aspect - it's stable.

If so and we should add auto-configuration for it, should we add a default ValueResolver that does String.valueOf instead of auto-configuring NoOpValueResolver?

Sure, why not, makes sense.