-
Notifications
You must be signed in to change notification settings - Fork 83
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
Migrate WebMvcTags
to ServerRequestObservationConvention
#586
Conversation
...3_0/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConventionTest.java
Outdated
Show resolved
Hide resolved
79f12c4
to
1270277
Compare
52d92f3
to
6f27d4e
Compare
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/spring/boot3/MigrateWebMvcTagsToObservationConvention.java
Outdated
Show resolved
Hide resolved
Explicitly add micrometer-observation jar
" public KeyValues getLowCardinalityKeyValues(ServerRequestObservationContext context) {\n" + | ||
" KeyValues values = super.getLowCardinalityKeyValues(context);\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you explore whether to use low cardinality versus high cardinality? It seems more risky to assume low cardinality. Any thoughts on that subject?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're very much correct in saying choosing the wrong one can cause big issues in the ingestion of traces/metrics.
It'd be safer to apply high and require manual shifting of high/low than adding all as low and potentially causing issues!
I'll change it to high!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test that fails just from having another method without any arguments, as that leads to class cast exception. Likely you'll need some form of limiting when to modify methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see; I understand this one wasn't easy with the lack of public examples and the number of options that needed to be converted. Thanks for diving in! Let's see how it holds up in practice with the coming release.
What's your motivation?
WebMvcTags
toServerRequestObservationConvention
#531Anyone you would like to review specifically?
@timtebeek
Any additional context
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0-Migration-Guide#tag-providers-and-contributors-migration
Checklist