-
Notifications
You must be signed in to change notification settings - Fork 38
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 instrumentation of driver_requests_* metrics #153
Conversation
@saley89 Thank you for your contribution. |
Thank you @saley89 for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
Hi @rishabh-11 my team has a need for the metrics that are implemented by this change and I noticed it was relatively straight forward to add these in based on the implementation in the other provider. If you are comfortable for this PR to go in for these metrics without the others that would be beneficial for us. The other metrics could go in as and when you have time/are able to do so. |
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.
Can you move the instrument.go
file in a separate directory called instrument
inside pkg
?
@rishabh-11 sure, that is done now. |
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.
just a minor change. rest is good. Also did you test this? Are the metrics working as expected?
Regarding tests I think it can be done. Looks like the way to go about it would be to leverage the testutils package of the client_go/prometheus package we already have in this repo. Some of example of using it are in the prometheus repo. I had a brief look this afternoon to see if we can introduce it to the existing My thinking would be ideally to get the metric, call methods like |
Hey @saley89, While testing your PR I found a bug in the code related to metrics. Can you refer to gardener/machine-controller-manager-provider-azure#130 and fix this PR? |
/ping @saley89 |
@saley89 ℹ️ please take some time to help rishabh-11 or redirect to someone else if you can't. |
@saley89 You need rebase this pull request with latest master branch. Please check. |
@rishabh-11 I was able to pull in the changes needed for the updated driver API function. I needed to revendor due to go mod changes to bring in the latest prometheus golang client. I will rebase with master to fix these conflicts. |
Co-authored-by: Rishabh Patel <66425093+rishabh-11@users.noreply.github.com>
e33d389
to
335170a
Compare
The rebase has got things back in order and this is now ready for your review again. Thanks |
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
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
What this PR does / why we need it:
In version v0.50.0 of MCM new metrics were added to record driver request durations and failures. It is for the provider's to implement the logic of updating these metrics at the appropriate points in the code base based on their driver activity with the provider.
This PR is based on the machine-controller-manager-provider-azure implementation of these metrics and adds the duration of requests and records any failures during these operations:
Which issue(s) this PR fixes:
Fixes #152
Special notes for your reviewer:
See the linked
machine-controller-manager-provider-azure
project and whether we are able to take theirinstrument.go
function as is.Release note: