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

Remove outdated warnings in docs for @Counted/@Timed #5736

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Dec 8, 2024

This PR removes outdated warnings in docs for @Counted and @Timed as they seem to be for the removed Spring Boot legacy module.

I can see some more related to Spring Boot application properties for meter registries (ex. AppOptics). I think we can replace them with links to Spring Boot reference docs for them (ex. AppOptics). I can try this with another PR if you agree with this direction.

@shakuzen
Copy link
Member

shakuzen commented Dec 9, 2024

Thanks for the pull request. For what it's worth, the note (if I remember correctly) was for Spring Boot's support of Micrometer's annotations without using the corresponding aspect (TimedAspect/CountedAspect). Maybe that also applied to the spring legacy module support too - I don't remember. In Spring Boot 2.x, you could use the annotations without those aspects configured if you were using it on Controller methods with a RequestMapping, for example. But you could not use it on an arbitrary bean's method. I think any support of the annotations without using the aspects has been removed from Spring Boot and the aspect needs to be (auto-)configured now. The aspect implementations don't know anything about Spring and hence it will work on arbitrary methods that the aspect can instrument.

With all that said, I still agree it makes sense to remove this outdated note which probably didn't belong in the Micrometer docs to begin with (except to the extent it applied to the spring legacy module we used to have).

https://docs.spring.io/spring-boot/reference/actuator/observability.html#actuator.observability.annotations

@shakuzen
Copy link
Member

shakuzen commented Dec 9, 2024

I can see some more related to Spring Boot application properties for meter registries (ex. AppOptics).

I thought I opened an issue related to this, but maybe not. In general, yes it would be better to not document a third-party library's (Spring Boot's) configuration properties in our documentation. However, it is a nice compact way to document the Config class methods indirectly. So I would ideally like to come up with a way to generate that automatically or at least ensure it is up-to-date with the current version of the Config class. A cheap and lazy way might be to link to the JavaDoc for the Config type instead of the Spring Boot configuration properties. A link is easier to miss and moves the info one more hop away from the documentation users are looking at, though. So I'm not sure what will be best or feasible/easy to do in the short term, but I agree it's definitely not ideal how it is now.

I think we can replace them with links to Spring Boot reference docs for them (ex. AppOptics).

We can, but we do have a bit of a dependency cycle type of issue since Spring Boot depends on us and not the other way around. What version of the Spring Boot docs do we link to from our docs? I guess it isn't an issue once we release our first patch release after the corresponding Spring Boot release goes GA.

@izeye
Copy link
Contributor Author

izeye commented Dec 9, 2024

@shakuzen Thanks for the feedback!

"Micrometer's Spring Boot configuration" (not "Spring Boot's Micrometer configuration") or "Micrometer's Spring Boot support" (not "Spring Boot's Micrometer support") made me think that it's for the legacy Spring Boot support module.

I guess they have been updated for Spring Boot's Micrometer support after its release.

What version of the Spring Boot docs do we link to from our docs?

I just thought that it's just an example of configuration binding, so it's okay to use links to the current version of Spring Boot.

@shakuzen
Copy link
Member

so it's okay to use links to the current version of Spring Boot.

Perhaps, but those links could become broken in new versions of the Spring Boot docs (e.g. when section headers are renamed or moved or removed), which is the inherent danger in linking to a non-specific version of the docs.

@shakuzen shakuzen merged commit ae1a843 into micrometer-metrics:1.13.x Dec 12, 2024
7 checks passed
@shakuzen shakuzen added the doc-update A documentation update label Dec 12, 2024
@shakuzen shakuzen added this to the 1.13.10 milestone Dec 12, 2024
@izeye izeye deleted the outdated-warnings branch December 12, 2024 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-update A documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants