-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Fix admission metrics bucket sizes #78608
Conversation
/assign @logicalhan |
/priority important-soon we would cherry-pick this into the 1.15 release even if it merged post 1.15 |
879f8e9
to
2f81a70
Compare
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
/retest |
/cc @brancz What do you think about these bucket sizes? |
@@ -33,7 +33,7 @@ const ( | |||
|
|||
var ( | |||
// Use buckets ranging from 25 ms to ~2.5 seconds. |
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.
The current buckets generated by the expression underneath are: [0.025 0.0625 0.15625 0.390625 0.9765625]
so the comment is actually still inaccurate.
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.
If we want to stay within the constraints of five buckets then my suggestion is {0.025, 0.05, 0.25, 1.0, 5.0}
. We should also update the comment to match the buckets.
I realize trace threshold is configurable. Not sure exponential bucket are great as they seem to be causing some confusion. Maybe something like {0.025, 0.1, 0.25, 0.5, 1, 5} |
I like this idea. Since the admission webhook default (and max allowed) timeout is 30s, maybe: {0.025, 0.1, 0.5, 1, 5, 30} ? |
2f81a70
to
0e098cb
Compare
/lgtm |
0e098cb
to
8fc51b5
Compare
Going with buckets: |
/lgtm Thanks Joe! |
8fc51b5
to
a4f0487
Compare
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
/lgtm |
/retest |
rebased |
a4f0487
to
084c525
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, liggitt 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 Review the full test history for this PR. Silence the bot with an |
When #72343 fixed admission metrics to use seconds instead of microseconds as the measured unit (which was totally my fault), the bucket sizes didn't get updated and so are off by 6 orders of magnitude. This fixes them.
/kind bug
/sig api-machinery
/cc @logicalhan @danielqsj @brancz @sttts @liggitt @cheftako