-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Introduce support for Uni return types in Micrometer annotations #22639
Conversation
/cc @ebullient |
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.
I made a small comment on the handling of completion stages which does not handle cancellation.
try { | ||
return ((CompletionStage<?>) context.proceed()).whenComplete((result, throwable) -> { | ||
recordCompletionResult(counted, commonTags, throwable); | ||
return ((CompletionStage<?>) context.proceed()).whenComplete(new BiConsumer<Object, Throwable>() { |
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.
The problem with this approach is that you don't catch the cancellation, which means you would not record if the operation is canceled. I believe it's fine if it's documented (like do not use completion stage/ completable future, ever).
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.
Hm... I didn't actually change this part, I only removed the lambda. So I'll let @ebullient fix it
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building c18cc05
Failures⚙️ Initial JDK 11 Build #- Failing: extensions/panache/hibernate-orm-panache-common/runtime
! Skipped: devtools/bom-descriptor-json docs extensions/panache/hibernate-orm-panache-common/deployment and 24 more 📦 extensions/panache/hibernate-orm-panache-common/runtime✖ |
@aloubyansky any idea where the failure is coming from?
This PR didn't change anything WRT bootstrap. |
@aloubyansky I wonder if this is related to #22336 |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 8d0a952
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: extensions/micrometer/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/quartz/deployment extensions/scheduler/deployment and 19 more 📦 extensions/micrometer/deployment✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/micrometer/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/quartz/deployment extensions/scheduler/deployment and 19 more 📦 extensions/micrometer/deployment✖
⚙️ JVM Tests - JDK 17 #- Failing: extensions/micrometer/deployment
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/quartz/deployment extensions/scheduler/deployment and 19 more 📦 extensions/micrometer/deployment✖
|
Yes, it looks like it. I'll try to reproduce and fix it. I'm wondering why I haven't seen this kind of failure before. I wanted to refactor the ApplicationArchive stuff too but thought of doing it is a separate PR. Feel free to revert my PR for now. |
Hm... I think it didn't fail this time... |
Ah, now there are different failures:
|
Fixes: #15601