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.

Sign up for GitHub

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

Jump to bottom

Integrate OpenTelemetry to the WebSockets Next extension #41956

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Integrate OpenTelemetry to the WebSockets Next extension #41956

merged 1 commit into from
Nov 1, 2024

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Jul 17, 2024 •
edited
Loading

OTel part of the : #39143

@gsmet gsmet requested review from geoand, mkouba and brunobat July 17, 2024 15:55
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@michalvavrik
Copy link
Member Author

Crickey, I just mentioned that #39143 (comment) wants messages per connection and destination. Missed it till now. I'll make this draft and try to address it ASAP.

@michalvavrik michalvavrik marked this pull request as draft July 17, 2024 16:31
@michalvavrik michalvavrik marked this pull request as ready for review July 20, 2024 20:26
@michalvavrik
Copy link
Member Author

I think this is ready for review.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@michalvavrik michalvavrik force-pushed the feature/ws-next-otel branch 2 times, most recently from 7c325ce to 22b00b4 Compare July 21, 2024 09:28
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
geoand
geoand reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Looks great!

@brunobat
Copy link
Contributor

Checking out the PR and Reviewing now... Will take some time. It's a biggie :)

@michalvavrik
Copy link
Member Author

Thanks @michalvavrik. I hope you find my comments useful. Please let me know if you have questions. I think you also need a test were tracing is disabled and no traces are produced.

I am grateful for your review, it's great learning opportunity. I didn't address one of you comments until I get more feedback. Apart of that, I believe this PR is ready for another round or review. Thanks

@michalvavrik
Copy link
Member Author

need a test were tracing is disabled and no traces are produced.

arg, I forgot to add that one, I'll add it tomorrow

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
mkouba
mkouba reviewed Oct 25, 2024
View reviewed changes
...main/java/io/quarkus/websockets/next/runtime/telemetry/AbstractWebSocketEndpointWrapper.java Outdated Show resolved Hide resolved
...t/runtime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/TelemetryConstants.java Outdated Show resolved Hide resolved
...untime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/ConnectionInterceptor.java Show resolved Hide resolved
...e/src/main/java/io/quarkus/websockets/next/runtime/telemetry/WebsocketTelemetryProvider.java Outdated Show resolved Hide resolved
...ain/java/io/quarkus/websockets/next/runtime/telemetry/WebsocketTelemetryProviderBuilder.java Outdated Show resolved Hide resolved
...c/main/java/io/quarkus/websockets/next/deployment/WebsocketTelemetryCustomizerBuildItem.java Outdated Show resolved Hide resolved
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
brunobat
brunobat reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

Almost there

.../src/main/java/io/quarkus/websockets/next/runtime/telemetry/TracesConnectionInterceptor.java Outdated Show resolved Hide resolved
...ain/java/io/quarkus/websockets/next/runtime/telemetry/TracesForwardingWebSocketEndpoint.java Outdated Show resolved Hide resolved
...ain/java/io/quarkus/websockets/next/runtime/telemetry/TracesForwardingWebSocketEndpoint.java Outdated Show resolved Hide resolved
brunobat
brunobat approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

It looks good to me.
Thanks very much for this @michalvavrik.
Great stuff!

@michalvavrik
Copy link
Member Author

Thanks for all the comments @brunobat, definitely looks better now

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
mkouba
mkouba reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

I think that it looks really good now. I added a few minor/nitpicking comments ;-).

...-next/deployment/src/main/java/io/quarkus/websockets/next/deployment/WebSocketProcessor.java Outdated Show resolved Hide resolved
...-next/deployment/src/main/java/io/quarkus/websockets/next/deployment/WebSocketProcessor.java Outdated Show resolved Hide resolved
...ions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/runtime/Endpoints.java Show resolved Hide resolved
...ain/java/io/quarkus/websockets/next/runtime/telemetry/TracesForwardingWebSocketEndpoint.java Show resolved Hide resolved
...e/src/main/java/io/quarkus/websockets/next/runtime/telemetry/WebSocketTelemetryRecorder.java Show resolved Hide resolved
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 1, 2024
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 1, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 311de33.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

You can consult the Develocity build scans.

@mkouba mkouba merged commit e158abc into quarkusio:main Nov 1, 2024
23 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 1, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 1, 2024
@mkouba
Copy link
Contributor

mkouba commented Nov 1, 2024

Merged, thanks @michalvavrik!

@michalvavrik michalvavrik deleted the feature/ws-next-otel branch November 1, 2024 10:17
@geoand
Copy link
Contributor

geoand commented Nov 1, 2024

Awesome stuff!

I added the noteworthy-feature label to this

@mkouba
Copy link
Contributor

mkouba commented Nov 1, 2024

Awesome stuff!

I added the noteworthy-feature label to this

FYI I believe that WS Next will be feature-complete at the time of 3.17 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand left review comments

@brunobat brunobat brunobat approved these changes

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/documentation area/tracing area/websockets release/noteworthy-feature triage/flaky-test
Projects
Quarkus Documentation
Status: Done
Milestone
3.17.0.CR1
Development

Successfully merging this pull request may close these issues.
5 participants
@michalvavrik @brunobat @gsmet @mkouba @geoand