-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
ccl: Fill in chart catalog with missed metrics #81739
ccl: Fill in chart catalog with missed metrics #81739
Conversation
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.
Feel free to squash the commits (i.e. take ownership of my commit); there's generally no need to preserve my authorship. I'm happy you picked this up so quickly! Only need to figure out 1schedule_metrics.go` and then if it passes CI, LGTM.
Reviewed 2 of 5 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gh-casper and @koorosh)
pkg/jobs/schedule_metrics.go
line 91 at r2 (raw file):
}), NumTotalStartedSchedules: metric.NewGauge(metric.Metadata{
Wait, why are we adding metrics? I think the changes in this file should go, we should make sure the chart catalog reflects the truth, not make the truth reflect the chart catalog :-)
2004f02
to
b176763
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.
The only thing, I cannot push directly to your PR #81038 (I'm not in code owners group) so squashed all changes in this PR. Thanks for the feedback!
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gh-casper and @tbg)
pkg/jobs/schedule_metrics.go
line 91 at r2 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Wait, why are we adding metrics? I think the changes in this file should go, we should make sure the chart catalog reflects the truth, not make the truth reflect the chart catalog :-)
ah, that was too naive to adjust everything to fit the chart catalog's state :)
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.
you could git commit --amend --reset-author
to have your name on it but I don't know if it's worth your trouble.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @gh-casper and @tbg)
8561101
to
6d8ab6d
Compare
TestChartCatalogMetrics is our main enforcement mechanism around the hygiene of the chart catalog. Concretely, it checks that all metrics are present in the catalog. However, its attempt to check the other direction, namely that any metric mentioned in the catalog actually (still) exists was thwarted because `GenerateCatalog` silently drops such metrics (and so the test didn't ever get to see the stale entries). This commit adds a `strict` flag to `GenerateCatalog` and removes all stale metrics from the catalog. Release note: None
6d8ab6d
to
1bcbdd2
Compare
bors r+ |
Build succeeded: |
Based on PR: #81038
Resolves: #81359
This change adds missing metrics to the chart catalog.