-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 metrics to Runtime SDK hook client #6675
🌱 Add metrics to Runtime SDK hook client #6675
Conversation
f754d0a
to
ec924f4
Compare
ec924f4
to
f90ed03
Compare
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 great - using it now to get a feel for how the e2e tests run!
Just a couple of nits around comments.
f90ed03
to
c3425b3
Compare
c3425b3
to
adfbe00
Compare
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.
General approach looks fine. Mostly questions/nits as I don't really remember the metric best practices.
adfbe00
to
5911bf0
Compare
I would take another look once CI is green or ping me otherwise if you want another review :) |
5911bf0
to
1f79f18
Compare
/retitle 🌱 Add metrics to Runtime SDK hook client |
1f79f18
to
ae78c78
Compare
/lgtm /assign @fabriziopandini |
@ykakarap @killianmuldoon If you have some time, would be good if you can review this PR |
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.
/lgtm
Will take a look at this today :) |
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.
/lgtm
I am not very familiar with metric and practices to follow. But the overall approach here looks good.
/lgtm |
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@chrischdi please rebase when you've got some time :) |
ae78c78
to
6ffd863
Compare
@chrischdi: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
6ffd863
to
e36b78f
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@sbueringer: new pull request created: #6755 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
Adds request related metrics to the Runtime SDK hook client which will be helpful to analyse request durations to Runtime SDK extensions.
Adds the following metrics:
capi_runtime_sdk_request_latency_seconds
, example:capi_runtime_sdk_requests_total
example:Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #6330
Additional info
Some prior art from client-go: