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.

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

k8s,metrics: Remove double k8s event accounting #26349

Conversation

dylandreimerink
Copy link
Member

@dylandreimerink dylandreimerink commented Jun 19, 2023

Resources increment the k8s event metrics themselves now. This causes duplicate metric accounting for k8s watchers that have been implemented in terms of resources and also do metrics tracking.

This PR removes the metrics tracking done by k8s watchers that are implemented with resources.

Fixed double metric accounting for k8s events 

@dylandreimerink dylandreimerink 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. labels Jun 19, 2023
@dylandreimerink dylandreimerink requested review from a team as code owners June 19, 2023 10:48
@dylandreimerink dylandreimerink marked this pull request as draft June 19, 2023 10:49
@dylandreimerink
Copy link
Member Author

Converting to draft, something did not quite go right, for some reasons this commit contains unrelated changes

@dylandreimerink dylandreimerink force-pushed the feature/remove-double-metric-accounting branch 2 times, most recently from 3605fed to aa5fd4f Compare June 19, 2023 11:09
@dylandreimerink
Copy link
Member Author

Resolved 😅 🙏

@dylandreimerink dylandreimerink marked this pull request as ready for review June 19, 2023 11:13
@jrajahalme
Copy link
Member

Go lint says there is an unused function:

  Error: operator/watchers/k8s_service_sync.go:35:6: func `k8sEventMetric` is unused (unused)
  func k8sEventMetric(scope, action string) {
       ^

@christarazi christarazi added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. labels Jun 21, 2023
@dylandreimerink dylandreimerink force-pushed the feature/remove-double-metric-accounting branch from aa5fd4f to 620f8bb Compare June 27, 2023 09:07
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

Is this a regression? If yes, can you add a "Fixes:" tag?

@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink
Copy link
Member Author

Is this a regression? If yes, can you add a "Fixes:" tag?

I introduced the bug myself in #26269, Jussi cought it like 2 seconds after the thing was merged, so I just made the fix without going through the process of making an issue

Resources increment the k8s event metrics themselves now. This causes
duplicate metric accounting for k8s watchers that have been implemented
in terms of resources and also do metrics tracking.

This commit removes the metrics tracking done by k8s watchers that are
implemented with resources.

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
@dylandreimerink dylandreimerink force-pushed the feature/remove-double-metric-accounting branch from 620f8bb to 04808f4 Compare June 29, 2023 09:15
@dylandreimerink
Copy link
Member Author

/test

@dylandreimerink dylandreimerink added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Jun 29, 2023
@dylandreimerink dylandreimerink merged commit dda1202 into cilium:main Jun 29, 2023
65 checks passed
@joamaki joamaki mentioned this pull request Jul 5, 2023
23 tasks
@joamaki joamaki 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 Jul 5, 2023
@jibi jibi 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 Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics Impacts statistics / metrics gathering, eg via Prometheus. 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. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants