-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: Enable emittance of response time metrics (achieving parity with Kratos) #2323
Conversation
a7c13b4
to
b3ec156
Compare
I'm trying to see the details on why the CircleCI tests are failing, but after authenticating I just get redirected to the CircleCI homepage. Is there something else I need to do to be able to view the details? |
It's failing due to a missing |
eb2f70a
to
158601c
Compare
@aeneasr thanks! fixed edit: I'm able to see the CircleCI now, will fix the remaining issues. |
@aeneasr fixed the issues, CI passing, ready for review :) |
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.
Awesome 🎉
Thank you for your contribution!
Hey @aeneasr, just wondering, when do you think you'd be able to publish a release containing this commit? I see that you made a release yesterday, so I just missed it. Would it be possible to release a minor version or patch containing this? We're hoping to take our Hydra-powered service into production in the next week and not having these metrics would be a blocker for us. Let me know at your convenience! Thanks for all your hard work! 😄 |
We just released one yesterday I think - but I can sneak another one in ;) |
@aeneasr thanks so much for being so responsive, you're awesome 😍 |
Thank you for the contribution :) |
Proposed changes
Currently, Kratos emits response time metrics that can be used to monitor latency as well as Requests Per Second. Kratos's metrics middleware looks like:
Currently, the equivalent in Hydra omits the first and last lines:
Consequently, Hydra doesn't emit the response time metrics that would enable us to monitor latency/RPS.
In our case, being able to monitor these metrics is an absolute necessity, and generally, it would seem desirable to have parity between Hydra and Kratos in this regard.
This PR adds the missing lines to Hydra to enable emittance of these key metrics and achieve parity with Kratos.
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further comments