Recording openstack metrics #46008
Recording openstack metrics #46008
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/L
|
/assign @anguslees |
NickrenREN
force-pushed
the
openstack-add-metric
branch
from
035ec7e to
fe0e4d0
Compare
k8s-github-robot
added
the
approved
There was a problem hiding this comment.
Thanks, I like being able to export these metrics.
I would like it even more if we can make the call-site syntax really minimal - that way we can put it everywhere without messing up the visual flow of the code much. Ideally something as simple as a one-letter wrapper that takes the metric name, and a closure - not sure if that's possible though with golang's lack of parametric types :(
| return "", "", err | ||
| } | ||
| timeToken := time.Since(startTime).Seconds() | ||
| recordOpenstackOperationMetric("create_v1_volume", timeToken, nil) |
There was a problem hiding this comment.
I think you should move all these to above the if err != nil handler. ie: remove the explicitly different calls in separate branches:
vol, err := volumes_v1.Create(...)
timeToken := time.Since(startTime).Seconds()
recordOpenstackOperationMetric("create_v1_volume", timeToken, err)
// rest is unchanged from original:
if err != nil {
return "", "", err
}
return vol.ID, vol.AvailabilityZone, nilThere was a problem hiding this comment.
Yes, but maybe we do not need to calc timetaken if err happened
There was a problem hiding this comment.
I'm very reluctant to add anything complex on error paths because they're ~never tested - and there's rarely much point optimising the error case either...
| return "", "", err | ||
| } | ||
| timeToken := time.Since(startTime).Seconds() |
There was a problem hiding this comment.
Is this meant to be timeTaken (taken vs token)?
There was a problem hiding this comment.
Yes，timeTaken. Sorry i will change that
|
Thanks for your review. @anguslees |
NickrenREN
force-pushed
the
openstack-add-metric
branch
from
fe0e4d0 to
b3bb74f
Compare
k8s-github-robot
added
size/M
|
@k8s-bot ok to test |
| Name: OpenstackOperationErrorKey, | ||
| Help: "Cumulative number of openstack Api call errors", | ||
| }, | ||
| []string{"openstack_operation"}, |
There was a problem hiding this comment.
Can we use naming convention proposed here - https://github.com/kubernetes/community/blob/master/contributors/design-proposals/cloudprovider-storage-metrics.md ? Which is, rename openstack_operation to request ?
add openstack operation metrics Add support for emitting metrics from openstack cloudprovider about storage operations.
NickrenREN
force-pushed
the
openstack-add-metric
branch
from
b3bb74f to
18852c5
Compare
|
kindly ping @anguslees @gnufied . Ready for review again |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: NickrenREN, gnufied
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
@NickrenREN: The following test(s) failed:
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. |
|
Automatic merge from submit-queue |
Automatic merge from submit-queue Recording openstack metrics add openstack operation metrics **Release note**: ```release-note Add support for emitting metrics from openstack cloudprovider about storage operations. ``` /assign @gnufied
add openstack operation metrics
Release note:
/assign @gnufied