Skip to content
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.

Sign up for GitHub

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

Jump to bottom

metrics: fix issue where logging err/warn metric is never updated. #29201

Merged
merged 2 commits into from
Dec 7, 2023
Merged

metrics: fix issue where logging err/warn metric is never updated. #29201

merged 2 commits into from
Dec 7, 2023

Conversation

tommyp1ckles
Copy link
Contributor

Because legacy metrics are now initialized in Hive, the logging hook was being set to the NoOpCounterVec instance.

This moves initializing the errors/warnings metric out of the NewLegacyMetrics function and provides a manual way to init metrics that must be initialized prior to Hive.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 15, 2023
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/fix-errorwarnings-hook branch 2 times, most recently from ae43425 to 44783b7 Compare December 2, 2023 06:04
@tommyp1ckles tommyp1ckles added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. release-blocker/1.15 This issue will prevent the release of the next version of Cilium. labels Dec 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 2, 2023
@tommyp1ckles tommyp1ckles marked this pull request as ready for review December 5, 2023 06:22
@tommyp1ckles tommyp1ckles requested review from a team as code owners December 5, 2023 06:22
chancez
chancez approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. Might be worth using a gauge for the metric, since it's never going to be changed after initialization, but that's orthogonal to this PR>

pippolo84
pippolo84 requested changes Dec 6, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Minus the double invoke, LGTM.

pkg/metrics/cell.go Show resolved Hide resolved
@tommyp1ckles tommyp1ckles added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Dec 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.5 Dec 6, 2023
@christarazi christarazi added area/metrics Impacts statistics / metrics gathering, eg via Prometheus. area/daemon Impacts operation of the Cilium daemon. area/operator Impacts the cilium-operator component sig/agent Cilium agent related. labels Dec 7, 2023
@tommyp1ckles
Copy link
Contributor Author

/test

1 similar comment
@tommyp1ckles
Copy link
Contributor Author

/test

pippolo84
pippolo84 approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

@tommyp1ckles
metrics: fix issue where logging err/warn metric is never updated.
5a615d6 
Because legacy metrics are now initialized in Hive, the logging hook was
being set to the NoOpCounterVec instance.

This moves initializing the errors/warnings metric out of the
NewLegacyMetrics function and provides a manual way to init metrics that
must be initialized prior to Hive.

Fixes: cilium#29525

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles
pkg/metrics: remove old todo.
2370974 
This comment is over 5 years old and no longer seems relevant.

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@tommyp1ckles
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 7, 2023
@aanm aanm added this pull request to the merge queue Dec 7, 2023
Merged via the queue into cilium:main with commit 83b87c4 Dec 7, 2023
61 checks passed
@nebril nebril added this to Needs backport from main in 1.14.6 Dec 11, 2023
@nebril nebril removed this from Needs backport from main in 1.14.5 Dec 11, 2023
@giorio94 giorio94 mentioned this pull request Dec 13, 2023
Merged
10 tasks
@giorio94 giorio94 added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Dec 13, 2023
@joestringer joestringer mentioned this pull request Dec 14, 2023
Merged
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Dec 14, 2023
@gentoo-root gentoo-root moved this from Needs backport from main to Backport done to v1.14 in 1.14.6 Jan 17, 2024
@gentoo-root gentoo-root mentioned this pull request Jan 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chancez chancez chancez approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. area/operator Impacts the cilium-operator component backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.15 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/agent Cilium agent related.
Projects
No open projects
1.14.6
Backport done to v1.14
cilium v1.14.6
Status: Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tommyp1ckles @chancez @pippolo84 @christarazi @aanm @giorio94