Integrate Micrometer with WebSockets Next #44379

michalvavrik · 2024-11-07T23:01:53Z

closes: WebSockets Next: OTel integration #39143

github-actions · 2024-11-07T23:42:12Z

🙈 The PR is closed and the preview is expired.

michalvavrik · 2024-11-08T09:22:28Z

Interesting that MicrometerWebSocketsOnBinaryMessageTest.testClientEndpoint_MultiTextReceived_MultiTextSent is flaky. IIRC I cut down timeout to match the timeout used by WS Next OTel tests. Either I need to increase it or tune some setting. I'll have a look when I will be addressing reviewer requests for change.

mkouba

Looks very good and the test coverage is really impressive...

...t/runtime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/SendingInterceptor.java

...-next/deployment/src/main/java/io/quarkus/websockets/next/deployment/WebSocketProcessor.java

...t/runtime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/SendingInterceptor.java

michalvavrik · 2024-11-12T13:19:25Z

Ah, I forgot about that flakiness (it never fails locally), I'll address it now.

mkouba · 2024-11-18T09:02:22Z

...t/runtime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/TelemetryConstants.java

+    /**
+     * Counts all the WebSockets server endpoint errors.
+     */
+    public static final String SERVER_MESSAGES_COUNT_ERRORS = "quarkus.websockets.server.messages.count.errors";


I wonder if this should be something like SERVER_ENDPOINT_COUNT_ERRORS = "quarkus.websockets.server.endpoint.count.errors" instead because it's not only about messages but about endpoint logic in general (IIUIC it mirros the @OnError functionality).

Yes, I agree with that. I think I called it differently before but changed it based on suggestions. They are not only error messages. I'll change it. We will see what @brunobat thinks.

mkouba · 2024-11-18T09:04:10Z

...t/runtime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/TelemetryConstants.java

+    /**
+     * Counts all the WebSockets client endpoint errors.
+     */
+    public static final String CLIENT_MESSAGES_COUNT_ERRORS = "quarkus.websockets.client.messages.count.errors";


CLIENT_ENDPOINT_COUNT_ERRORS = "quarkus.websockets.client.endpoint.count.errors"??

...t/runtime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/TelemetryConstants.java

...nsions/websockets-next/runtime/src/main/java/io/quarkus/websockets/next/TelemetryConfig.java

...t/runtime/src/main/java/io/quarkus/websockets/next/runtime/telemetry/TelemetryConstants.java

michalvavrik · 2024-11-18T14:43:45Z

It's bit difficult as MicrometerWebSocketsOnTextMessageTest haven't failed locally yet, but I'll try to read code in next 2 days and figure why it could be flaky. There is probably wrong expected count somewhere.

mkouba · 2024-11-18T15:27:37Z

It's bit difficult as MicrometerWebSocketsOnTextMessageTest haven't failed locally yet, but I'll try to read code in next 2 days and figure why it could be flaky. There is probably wrong expected count somewhere.

I can try it locally too...

quarkus-bot · 2024-11-19T10:06:07Z

Status for workflow `Quarkus CI`

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

✅ 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.

You can consult the Develocity build scans.

quarkus-bot · 2024-11-19T10:07:42Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit d82340b.

✅ 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.

mkouba · 2024-11-19T10:08:06Z

This PR is ready for merge from my point of view! Let's wait for @brunobat though.

mkouba · 2024-11-21T10:27:57Z

If I understand it correctly the only thing we're waiting for is an integration test, right? If so, I'd like to merge this PR and address this last thing in a follow-up.

WDYT? @brunobat @michalvavrik

michalvavrik · 2024-11-21T10:56:05Z

If I understand it correctly the only thing we're waiting for is an integration test, right? If so, I'd like to merge this PR and address this last thing in a follow-up.

I am fighting with time, but I hope (once again) I could have time to add IT today (I also thought that yesterday :-D). Anyway, I think we are waiting for @brunobat review. Last time we talked Bruno was testing this PR locally and told me he needs more time.

michalvavrik · 2024-11-21T10:56:57Z

address this last thing in a follow-up.

yeah, that part alone can be done easily separately

brunobat

In general I think the tests are very thorough, however thay are also a bit opaque because of the methods inside MetricsAsserter. Future debugging, might force us to refactor that.

To be fair, all this instrumentation should have been done in the quarkus-micrometer project under io.quarkus.micrometer.runtime.binder.
Same for OpenTelemetry.
We shouldn't be pulverising the extensions with micrometer and OTel libs because it will make maintenance a pain.

In any case, having the functionality is more important. Do you think this could be moved in the future?

What do you think @michalvavrik?
... And sorry for not thinking of this before.

michalvavrik · 2024-11-21T14:46:39Z

To be fair, all this instrumentation should have been done in the quarkus-micrometer project under io.quarkus.micrometer.runtime.binder. Same for OpenTelemetry. We shouldn't be pulverising the extensions with micrometer and OTel libs because it will make maintenance a pain.
Do you think this could be moved in the future?

If I unseal SendingInterceptor, ErrorCountingInterceptor and ConnectionInterceptor, then moving it should be as easy as moving this code to the quarkus-micrometer and adding conditional WS-Next dependency to quarkus-micrometer or introducing runtime SPI. Same should be true for the OTel integration because I believe there is no tight coupling. I am happy to do that, but maybe it would be easier if we got this in and follow up with moving the code and IT?

brunobat

Thanks very much for this @michalvavrik !

brunobat · 2024-11-21T16:18:34Z

To be fair, all this instrumentation should have been done in the quarkus-micrometer project under io.quarkus.micrometer.runtime.binder. Same for OpenTelemetry. We shouldn't be pulverising the extensions with micrometer and OTel libs because it will make maintenance a pain.
Do you think this could be moved in the future?

If I unseal SendingInterceptor, ErrorCountingInterceptor and ConnectionInterceptor, then moving it should be as easy as moving this code to the quarkus-micrometer and adding conditional WS-Next dependency to quarkus-micrometer or introducing runtime SPI. Same should be true for the OTel integration because I believe there is no tight coupling. I am happy to do that, but maybe it would be easier if we got this in and follow up with moving the code and IT?

Deal!

mkouba · 2024-11-21T16:41:28Z

and adding conditional WS-Next dependency to quarkus-micrometer

👎

or introducing runtime SPI.

👍

* Adding tests for Micrometer metrics exposed for WebSockets. Test plan: https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QUARKUS-5667.md Feature issue: https://issues.redhat.com/browse/QUARKUS-5667 Code changes: quarkusio/quarkus#44379

michalvavrik requested review from brunobat and mkouba November 7, 2024 23:02

quarkus-bot bot added the area/documentation label Nov 7, 2024

michalvavrik added area/metrics area/websockets labels Nov 7, 2024