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

feat: Enable emittance of response time metrics (achieving parity with Kratos) #2323

Merged
merged 4 commits into from
Jan 28, 2021

Conversation

mbonnell-wish
Copy link
Contributor

@mbonnell-wish mbonnell-wish commented Jan 26, 2021

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:

func (pmm *MetricsManager) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
	start := time.Now()
	next(rw, r)

	pmm.prometheusMetrics.ResponseTime.WithLabelValues(r.URL.Path).Observe(time.Since(start).Seconds())
}

Currently, the equivalent in Hydra omits the first and last lines:

func (pmm *MetricsManager) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
	next(rw, r)
}

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

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    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.
  • [] I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2021

CLA assistant check
All committers have signed the CLA.

@mbonnell-wish mbonnell-wish changed the title Enable emittance of response time metrics (achieving parity with Kratos) fix: Enable emittance of response time metrics (achieving parity with Kratos) Jan 26, 2021
@mbonnell-wish mbonnell-wish changed the title fix: Enable emittance of response time metrics (achieving parity with Kratos) feat: Enable emittance of response time metrics (achieving parity with Kratos) Jan 26, 2021
@mbonnell-wish
Copy link
Contributor Author

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?

@aeneasr
Copy link
Member

aeneasr commented Jan 26, 2021

It's failing due to a missing "time" import :)

@mbonnell-wish
Copy link
Contributor Author

mbonnell-wish commented Jan 26, 2021

@aeneasr thanks! fixed

edit: I'm able to see the CircleCI now, will fix the remaining issues.

@mbonnell-wish
Copy link
Contributor Author

@aeneasr fixed the issues, CI passing, ready for review :)

Copy link
Member

@aeneasr aeneasr left a 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!

@aeneasr aeneasr merged commit c1f1ba5 into ory:master Jan 28, 2021
@mbonnell-wish
Copy link
Contributor Author

mbonnell-wish commented Jan 29, 2021

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! 😄

@mbonnell-wish mbonnell-wish deleted the metrics-kratos-parity branch January 29, 2021 03:50
@aeneasr
Copy link
Member

aeneasr commented Jan 29, 2021

We just released one yesterday I think - but I can sneak another one in ;)

@aeneasr
Copy link
Member

aeneasr commented Jan 29, 2021

@mbonnell-wish
Copy link
Contributor Author

@aeneasr thanks so much for being so responsive, you're awesome 😍

@aeneasr
Copy link
Member

aeneasr commented Jan 31, 2021

Thank you for the contribution :)

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.

4 participants