-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add GRPC client and server metrics #5615
Add GRPC client and server metrics #5615
Conversation
75f99de
to
d6752e7
Compare
Could you add some unit tests/integration tests for this feature? |
Oh yes, for sure. I found the metrics tests, I will add checks for these there. |
@SpiritZhou Can the tests run in Kind? We don't use Helm on our clusters nor do we use kubernetes networking, so I have to run locally. |
Hello! |
/run-e2e prometheus_metrics |
@JorTurFer can you rerun the prom tests? I set the family name incorrectly in the tests and that's why the histogram tests failed. |
4bd0283
to
ea048e3
Compare
lol our security tools won't let me run @JorTurFer can you rerun the prometheus e2e tests? |
/run-e2e prometheus_metrics |
I ended up removing the test for the histogram, there are three metrics in that family and that's causing the crash so I'm just going to assume if the other 4 metrics are there it's there as this is upstream metrics functionality and not something we're reporting to our selves. |
@JorTurFer lol, can you run the e2e tests one more time :) |
/run-e2e prometheus_metrics |
8e7ae09
to
124fda8
Compare
@JorTurFer Can you run e2e on these new changes? |
/run-e2e prometheus_metrics |
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.
This is a great addition. My only concern is about naming of these metrics. The grpc
there is too generic. Maybe keda_metricservice_
or something around that?
I can make that change. |
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
022b8f0
to
65ef5c4
Compare
Omg, I am very sorry, the prefix should be |
Haha all good, will fix it quickly! |
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
Updated the name! Can you re-run e2e tests when they're ready? |
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
Signed-off-by: Ali Aqel <aliaqel@stripe.com>
@zroubalik I can't see the snyk test failure because I don't have access to their UI |
/run-e2e prometheus_metrics |
that's fine |
This PR adds GRPC Client and Server interceptors that emit GRPC request metrics from the perspective of the Client and Server. This allows developers to observe the performance of the adapter -> operator grpc connection.
To implement this, I use the upstream grpc-middleware with default prometheus metrics. I create helpers to create the metrics objects in
metricscollector
and then add them to the client/server if prometheus metrics are enabled.Tested on cluster, screenshot from Grafana:
Checklist
Fixes #5502
Relates to kedacore/keda-docs#1340