-
Notifications
You must be signed in to change notification settings - Fork 120
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
Introduce new metrics to track API call duration and Update status.Status to capture underlying cause #842
Conversation
@himanshu-kun, @rishabh-11 You have pull request review open invite, please check |
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.
thanks for the changes!
minor nits requested.
/assign @unmarshall to address the review |
changed the name of the API request duration metric added metric to capture overall duration for driver api calls reverted back to using service label for provider api metric added cause to status.Status added WrapError function corrected docstrings corrected leftover docstring Update pkg/util/provider/metrics/metrics.go Co-authored-by: Himanshu Sharma <[email protected]> Apply suggestions from code review Co-authored-by: Himanshu Sharma <[email protected]> addressed review comment - removed populating cause when in FromError when err is already status.Status
@rishabh-11 You have pull request review open invite, please check |
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:
Two new metrics have been introduced
In addition to the metrics,
status.Status
has been enhanced to capture the underlinecause error
. A convenience functionWrapError
has also been added which captures the underline cause. This will help consumers(mcm provider implementations) to introspect the error to extract underline error code, especially during writing unit testsWhich issue(s) this PR fixes:
Fixes #841
Special notes for your reviewer:
Release note: