-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
internal/store/pod.go: Only create waiting_reason series if pods are in waiting state #1378
internal/store/pod.go: Only create waiting_reason series if pods are in waiting state #1378
Conversation
waiting. This reduces the cardinality of this metric greatly, as it was one of the highest cardinality metrics pre 2.0.
Seems like CI is not happy, but strategy and implementation is sound. /lgtm |
cf137a6
to
9d147ca
Compare
Thanks @brancz I always forget about our main tests :D PTAL, thanks! /hold Put a hold until one of the reporters confirms this fixes it 👍 |
/lgtm |
As with 2.0 we are breaking support for 1.x and previous stabilities. In case we run into problems this gives us a chance to revert this.
61a3881
to
93aeadc
Compare
Looks great, thanks! |
/hold cancel @brancz PTAL, I adjusted the docs so lgtm got dropped. |
LGTM |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brancz, lilic 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 |
Due to the extremely high cardinality of this metric and with 2.0 release being breaking, this now changes that the waiting_reason metrics only include series that have state
Waiting
. We also now can include all the reasons as cardinality will only always be O(count of containers waiting). I made the adjustment for both containers and init containers.The reduction of series can be seen in the unit tests, but I also tried it on my cluster and this is the result we only see the waiting containers:
Will open PRs to adjust the 0 values for other metrics here as well in another PR.
@brancz @smarterclayton @vsxen can you please have a look if this metric continues to work as you want.
Fixes #1321