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

fix: request metrics #1007

Merged
merged 13 commits into from
Sep 13, 2022
Merged

fix: request metrics #1007

merged 13 commits into from
Sep 13, 2022

Conversation

nipsufn
Copy link
Contributor

@nipsufn nipsufn commented Sep 6, 2022

http_request_* metrics contain data related only to /metrics/prometheus endpoint.
This PR adds endpoints from non-monitoring routers.

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code 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
    [email protected]) 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

@hperl
Copy link
Collaborator

hperl commented Sep 6, 2022

Thanks for the PR! Do you think it makes sense to add the tracing for gRPC as well, or was that already enabled?

@nipsufn
Copy link
Contributor Author

nipsufn commented Sep 7, 2022

GRPC request metrics are not implemented yet, I'll see about adding them.

@nipsufn nipsufn marked this pull request as draft September 7, 2022 05:32
Copy link
Collaborator

@hperl hperl left a comment

Choose a reason for hiding this comment

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

Hi, thanks, looks good for HTTP traffic. In Cloud Keto is 100% HTTP/REST right now, but we want to change that in the future.

Do we have any way to test that the Prometheus metrics are correctly exposed here? Or are the E2E tests in Cloud anyways?

@nipsufn
Copy link
Contributor Author

nipsufn commented Sep 8, 2022

I've added test similar to those we have in other applications. More metric related tests are done on x repo, I think this should be OK for what we are doing.

@nipsufn nipsufn marked this pull request as ready for review September 8, 2022 11:00
@hperl hperl merged commit 96ff767 into ory:master Sep 13, 2022
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.

2 participants