-
Notifications
You must be signed in to change notification settings - Fork 3k
WebSockets Next: use SPI to separate the Websockets extension from OpenTelemetry and Micrometer extensions #45522
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
WebSockets Next: use SPI to separate the Websockets extension from OpenTelemetry and Micrometer extensions #45522
Conversation
michalvavrik
commented
Jan 13, 2025
- Integrate Micrometer with WebSockets Next #44379 (review)
/cc @brunobat (micrometer,opentelemetry), @ebullient (micrometer), @radcortez (opentelemetry) |
🎊 PR Preview dd7edc6 has been successfully built and deployed to https://quarkus-pr-main-45522-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Looks good but I cannot talk about the opentelemetry/micrometer part..
...ry/deployment/src/main/java/io/quarkus/opentelemetry/deployment/tracing/TracerProcessor.java
Outdated
Show resolved
Hide resolved
...loyment/src/main/java/io/quarkus/micrometer/deployment/binder/WebSocketsBinderProcessor.java
Outdated
Show resolved
Hide resolved
139641c
to
8d40ce1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hey @brunobat , could you have a look at this one? It looks like something we would want for the next LTS. |
I wish we had discussed this before implementing it because if we adopt the observation API in the future, we will need to rewrite it all again. Maybe we did, but I don't remember. Sorry. |
I don't mind rewriting it again @brunobat . |
no, we didn't, I didn't realize this could be improved, I am happy to meet you and rewrite this. Thanks |
@brunobat any news? |
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've decided this is good to go.
We will need to handle whatever happens after Observability.next, afterwards.
8d40ce1
to
151ee9c
Compare
I rebased the PR to fix a conflict and get an updated CI run. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Looks ok, however, we need to verify that:
- This is not a breaking change
- There are no split packages
I'll check early next week. |
Makes sense, but I'm intrigued why we haven't seen this failure sooner... |
1d7326a
to
4f52cf2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The failure is gone, |
I have requested 2 other reviews because it's unclear to me what is current situation (what are we waiting for with merging). |
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.
...io/quarkus/micrometer/runtime/binder/websockets/WebSocketMetricsInterceptorProducerImpl.java
Show resolved
Hide resolved
...src/main/java/io/quarkus/websockets/next/runtime/spi/telemetry/WebSocketEndpointContext.java
Outdated
Show resolved
Hide resolved
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 WS Next part is ok. I just added a tiny nitpicking comment ;-).
...ain/java/io/quarkus/websockets/next/runtime/telemetry/TelemetryWebSocketEndpointContext.java
Show resolved
Hide resolved
4f52cf2
to
1e3128a
Compare
This comment has been minimized.
This comment has been minimized.
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.
Thanks @michalvavrik
This comment has been minimized.
This comment has been minimized.
1e3128a
to
d1e7009
Compare
GH CI stuck on "CI Sanity Check" (Expected - Waiting for status to be reported). I'll rebase again. |
d1e7009
to
ca75c71
Compare
Status for workflow
|
Status for workflow
|