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
Preserve legacy inflight metrics and fixes registration #88609
Preserve legacy inflight metrics and fixes registration #88609
Conversation
514eb12
to
1165647
Compare
Can you add a |
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.
I would say that the change here is not "fixes registration" but rather "uses the standard metrics abstraction instead of going directly to Prometheus".
@@ -57,6 +55,8 @@ func GetClassification(ctx context.Context) *PriorityAndFairnessClassification { | |||
return ctx.Value(priorityAndFairnessKey).(*PriorityAndFairnessClassification) | |||
} | |||
|
|||
var atomicMutatingLen, atomicNonMutatingLen = atomic.NewInt32(0), atomic.NewInt32(0) |
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.
Why use a third party package (go.uber.org/atomic) when we can get the functionality we are using here from sync/atomic?
/assign @logicalhan |
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
Need to run golint though. |
And we agreed to replace the use of uber's atomic with golang's atomic. |
@yue9944882 : you have just a couple of tiny things to do here. As far as I can tell, the import statement that golint complained about can simply be deleted. See MikeSpreitzer@52c1ee5 . I tested that, and it seems to work. |
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
/assign @lavalamp |
/priority important-soon |
/retest |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, yue9944882 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 |
/sig api-machinery
/kind feature
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: