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

Bump micrometer version to latest #829

Merged
merged 3 commits into from Oct 16, 2023
Merged

Conversation

dsyer
Copy link
Contributor

@dsyer dsyer commented Jan 20, 2023

Fixes #828

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jan 20, 2023

I'm unsure about this change.
AFAICT it doesn't provide any benefit for the user, except that they don't have to specify a newer version of micrometer themselves (since the used APIs didn't change at all).
But if they were changed, then we would loose backwards compatibility with 2.7 🤔
It also (very little) increases the maintenance required to keep the version up to date.

@ST-DDT ST-DDT added feedback required Information are missing or feedback for suggestions is requested dependencies Dependency updates labels Jan 20, 2023
@dsyer
Copy link
Contributor Author

dsyer commented Jan 21, 2023

It's a step in the direction of supporting Spring Boot 3. Whether this tiny change is a good idea or not really hinges on the approach the project maintainers want to take to that. If you want a smooth transition for users I think it should be possible to support Spring Boot 3 and 2.7 with a single release, and one of the steps required will be to upgrade Micrometer, while keeping support for older versions. This is a baby step in that direction. And that is the direction that has kind of been indicated already with the addition of the new style .imports metadata for autoconfiguration. If you prefer a different approach maybe you could outline it here or in one of the other issue discussions.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jan 21, 2023

and one of the steps required will be to upgrade Micrometer,

Thats the point I dont understand. Why is it required to update micrometer?
Do you have an error or incompatility here?

@dsyer
Copy link
Contributor Author

dsyer commented Jan 21, 2023

Because Spring Boot 3 uses micrometer features not available before 1.10, and because Spring Cloud Sleuth no longer exists and has effectively been replaced with Micrometer 1.10. Without this upgrade there’s no path for Spring Boot users to get tracing when they migrate to 3.0.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jan 21, 2023

Without this upgrade there’s no path for Spring Boot users to get tracing when they migrate to 3.0.

If they use the spring boot v3 parent bom, then they will automatically have the micrometer update. And if they dont, they can still update micrometer themselves. Depending on the order they are importing the dependencies in they might even get the micrometer update anyway.
Am I missing something?

@dsyer
Copy link
Contributor Author

dsyer commented Jan 21, 2023

Apparently. There’s no more sleuth so we need to add micrometer tracing for grpc to this library. There’s no way to do that without an upgrade, but it can be done in such a way that existing Spring Boot 2.7 apps won’t be affected (I believe).

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jan 21, 2023

need to add micrometer tracing for grpc to this library

Since it is an optional dependency the user has to add it to their dependencies anyway.

@dsyer
Copy link
Contributor Author

dsyer commented Jan 21, 2023

It’s an optional dependency but this library contains autoconfiguration for tracing, and users will expect that the same level of feature will be available for Spring Boot 3 as with Spring Cloud Sleuth. Again, this requires an upgrade of Micrometer.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jan 21, 2023

So instead of just upgrading micrometer you want to add some bean config as well to automatically setup the tracing beans?
Is there a reason, why you didnt include that in this PR?

@dsyer
Copy link
Contributor Author

dsyer commented Jan 22, 2023

I think that’s the best way forward, but you’re the one making decisions. I’m not going to propose any configuration changes until we agree it’s a good idea. I didn’t include those changes partly because I wanted to sound you out first, and partly because there are other blockers for Spring Boot 3 migration (SocketUtils at least, possibly others).

@dsyer
Copy link
Contributor Author

dsyer commented Jan 24, 2023

I added a new commit with some autoconfiguration changes. I tested the server configuration in a sample. There's no tracing sample in this project and also no Spring Boot 3 sample, so I didn't want to add that too at the same time.

@dsyer dsyer force-pushed the micrometer branch 4 times, most recently from 33d6e33 to 78486b2 Compare January 26, 2023 07:48
@dsyer
Copy link
Contributor Author

dsyer commented Feb 6, 2023

This is still marked as "waiting for feedback" but I'm not sure what is needed. Can we move this forward please?

@ST-DDT ST-DDT removed the feedback required Information are missing or feedback for suggestions is requested label Feb 6, 2023
@ST-DDT
Copy link
Collaborator

ST-DDT commented Feb 6, 2023

Can we move this forward please?

I'm currently busy and don't have time to work on this project. 😿

@yidongnan
Copy link
Collaborator

yidongnan commented Sep 9, 2023

I read the discussion above and it looks like this change will make it incompatible with spring boot 2.7, is it possible to make it compatible with both 2.7 and 3.0 while tracking is working?

@dsyer
Copy link
Contributor Author

dsyer commented Sep 9, 2023

I think you misunderstood. The whole point is to make Spring Boot 3 and 2.7 work (you still have to pick one and add the right dependencies).

@yidongnan
Copy link
Collaborator

I added a new commit with some autoconfiguration changes. I tested the server configuration in a sample. There's no tracing sample in this project and also no Spring Boot 3 sample, so I didn't want to add that too at the same time.

Sorry, I'm not very familiar with spring boot 3.0 at the moment and I see compatibility issues mentioned here.

I understand that micrometer was introduced to enable spring boot 3.0 applications to support grpc tracing, and if spring boot 2.7 is used and tracing still works (using spring cloud sleuth), then I think this PR is very valuable. Is there something I am missing?

@dsyer
Copy link
Contributor Author

dsyer commented Sep 11, 2023

Don't think so. That pretty much sums it up - with Spring Boot 2.7 you need Sleuth and with Spring Boot 3.0 it's just Micrometer.

@yidongnan yidongnan added this to the 3.0.0 milestone Oct 10, 2023
yidongnan added a commit that referenced this pull request Oct 10, 2023
* In the future, support the Trace feature by merging #829
@yidongnan yidongnan mentioned this pull request Oct 10, 2023
4 tasks
@yidongnan
Copy link
Collaborator

Would you be able to take some time to merge the master branch and update it?

@yidongnan yidongnan self-requested a review October 11, 2023 02:29
This will activate in a Spring Boot 3 application if there is a
dependency on one of the micrometer bridge jars, e.g.
io.micrometer:micrometer-tracing-bridge-{otel,brave}.
@dsyer
Copy link
Contributor Author

dsyer commented Oct 11, 2023

I rebased and the tests are green. Haven't looked into the details much.

Copy link
Collaborator

@yidongnan yidongnan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have tested it, and it appears to be working as expected.

@yidongnan yidongnan merged commit b0ceb98 into grpc-ecosystem:master Oct 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Dependency updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Micrometer to latest release
3 participants