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.

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

Set request context before recording metrics #5356

Conversation

ahus1
Copy link
Contributor

@ahus1 ahus1 commented Oct 13, 2024

Closes #5355

Motivation:

This sets the context so that the logic in Quarkus will know that this call is traced via OpenTracing, and then records and exemplar for the metrics.

I've been monkey-patching Keycloak 26 with a changed Http1xServerConnection#reportResponseComplete() and it worked for me in in a manual test.

I've been looking for other places of metrics.requestReset() and metrics.responseEnd() and fixing them the same way.

Draft PR for now, as I didn't add any tests, and would probably need help writing some if they would be required. Also waiting for a results that the build returns.

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

@vietj vietj added this to the 5.0.0 milestone Oct 14, 2024
@ahus1 ahus1 marked this pull request as ready for review October 16, 2024 13:24
@ahus1
Copy link
Contributor Author

ahus1 commented Oct 16, 2024

I've added a reproducer in https://github.com/ahus1/quarkus-otel-missing-examplar-example

Not sure how to best add a test case for that. I'd love to hear a hint on how to do that.

@tsegismont
Copy link
Contributor

@ahus1 see my comments in quarkusio/quarkus#43845 and #5355 (rescheduling the task on the context bound to the request is not a good option).

@ahus1
Copy link
Contributor Author

ahus1 commented Oct 19, 2024

Changing this to draft, and will probably close it as it is likely to be handled on the Quarkus side

@ahus1 ahus1 marked this pull request as draft October 19, 2024 09:43
@ahus1
Copy link
Contributor Author

ahus1 commented Oct 22, 2024

Closing as it will be handled on the Quarkus side. Sorry for the noise!

@ahus1 ahus1 closed this Oct 22, 2024
@tsegismont tsegismont removed this from the 5.0.0 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Http1xServerConnection doesn't set the Vert.x context when recording metrics
3 participants