-
Notifications
You must be signed in to change notification settings - Fork 159
Refactor metric defer() statements to gRPC metric interceptor #1876
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
Refactor metric defer() statements to gRPC metric interceptor #1876
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pwschuurman 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 |
/retest |
1 similar comment
/retest |
/assign @amacaskill |
@pwschuurman Can you please add a release note? The release note in the PR is how changes show up in the release notes/changelog that is generated when you press the generate release notes button when cutting a release. I think this is useful to know what version had what change, especially for SLO changes when we are debugging to see if whatever cluster triggered the alert, has your fix. |
a50040a
to
933b71a
Compare
933b71a
to
0a74064
Compare
/hold Tests added but want to add a regression test for one of the rpc calls that invoke the |
0a74064
to
e0184d2
Compare
…lows for more accurate error code reporting if gRPC functionality is refactored
e0184d2
to
a1ed4e4
Compare
/unhold |
/lgtm |
/cherry-pick release-1.14 |
@pwschuurman: #1876 failed to apply on top of branch "release-1.14":
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-sigs/prow repository. |
/cherry-pick release-1.15 |
@pwschuurman: new pull request created: #1886 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-sigs/prow repository. |
This fixes a regression introduced in kubernetes-sigs#1876 where the driver would start panicking on startup if `--http-endpoint` was specified. This was caused by the metrics not being initialized anymore during startup. The proposed fix involves using the `Reset` methods of the metrics object instead of trying to redefine them each time they need to be reset.
This fixes a regression introduced in kubernetes-sigs#1876 where the driver would start panicking on startup if `--http-endpoint` was specified. This was caused by the metrics not being initialized anymore during startup. The proposed fix involves using the `Reset` methods of the metrics object instead of trying to redefine them each time they need to be reset.
This fixes a regression introduced in kubernetes-sigs#1876 where the driver would start panicking on startup if `--http-endpoint` was specified. This was caused by the metrics not being initialized anymore during startup. The proposed fix involves using the `Reset` methods of the metrics object instead of trying to redefine them each time they need to be reset.
Migrate metric defer() statements to gRPC metric interceptor. This allows for more accurate error code reporting of the internal
operation_errors
metric when gRPC handling functionality is refactored/kind cleanup
What this PR does / why we need it: Currently the ControllerServer reports error metrics through a
defer()
function pattern. This captures the output of captured variables when the function returns. If captured references are not updated (eg: a statement is refactored, and theerr
variable is not updated), the error code reported may not be accurate. This can result in poor error classification when this metric is used for reporting (as all error codes may be classified asINTERNAL
).Special notes for your reviewer:
Does this PR introduce a user-facing change?: