Refactor common component metrics #11184

machadovilaca · 2024-02-07T19:16:09Z

What this PR does

Before this PR:

Old metric implementation style for client, workqueue and reflector metrics

After this PR:

According to kubevirt/community#219, refactor client, workqueue and reflector metrics to follow new approach

jira-ticket: https://issues.redhat.com/browse/CNV-27306
jira-ticket: https://issues.redhat.com/browse/CNV-38135
jira-ticket: https://issues.redhat.com/browse/CNV-38136

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

none

machadovilaca · 2024-02-07T19:16:34Z

/cc @enp0s3

machadovilaca · 2024-02-12T17:26:51Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

machadovilaca · 2024-02-14T11:19:56Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

machadovilaca · 2024-02-14T14:30:58Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

enp0s3

@machadovilaca Hi, thanks for the PR, looks good.
I saw that you've added new files, can you please change the naming style to snake case instead of camel case? its for consistency.

enp0s3 · 2024-02-15T16:35:37Z

tools/doc-generator/doc-generator.go

@@ -184,6 +172,31 @@ func getMetricsNotIncludeInEndpointByDefault() metricList {
 	return metrics
 }

+func getVirtControllerMetrics() metricList {
+	err := virt_controller.SetupMetrics(nil, nil, nil, nil, nil, nil, nil, nil)
+	checkError(err)


checkError is really redundant, it doesn't to the coding style IMO, there is no added value in calling a dedicated function for that.

it was previously done in this script, do you think I can/makes sense to edit the whole file and remove the checkErrors?

enp0s3 · 2024-02-15T16:38:08Z

tools/prom-metrics-collector/metrics_collector.go

@@ -33,7 +33,10 @@ import (
 // https://sdk.operatorframework.io/docs/best-practices/observability-best-practices/#metrics-guidelines
 // should be ignored.
 var excludedMetrics = map[string]struct{}{
-	"kubevirt_vmi_phase_count": struct{}{},
+	"kubevirt_vmi_phase_count":                  {},


Why do you want to ignore them?

this collector is used by the linter, and these metrics do not follow the naming conventions, that's why I'm excluding them from the linter, while we don't refactor them

maybe worth changing the names to start with kubevirt_?

I agree we need to do so, but I don't think it should be as part of this PR

@machadovilaca Please add kubevirt_ prefix to all the metrics.
You already added reflector_ to the reflector metrics.

I'm avoiding renaming any metrics as part of this PRs
reflector was already part of the metrics names through the 'subsytem':

kubevirt/pkg/monitoring/reflector/prometheus/prometheus.go

Line 26 in 055c6e0

const reflectorSubsystem = "reflector"

I'm also avoiding using the subsystem method because of this, IMO it makes it harder to find metrics by name and it is easier to generate confusion

machadovilaca · 2024-02-20T17:20:56Z

/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring

machadovilaca · 2024-02-20T17:21:02Z

@enp0s3 updated

acardace · 2024-04-03T14:11:22Z

/approve

kubevirt-bot · 2024-04-03T14:11:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acardace

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:

~~OWNERS~~ [acardace]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-commenter-bot · 2024-04-03T14:11:45Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

kubevirt-commenter-bot · 2024-04-03T21:52:37Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-04-04T07:52:40Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-04-04T13:52:41Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-04-05T00:52:53Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-04-05T03:53:50Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2024-04-05T06:55:39Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fossedihelm · 2024-04-05T07:34:45Z

/hold
monitoring failure is relevant
@machadovilaca

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca · 2024-04-08T12:49:35Z

/hold monitoring failure is relevant @machadovilaca

fails due to the merge of #11307
reflector metrics were not being created for a while now, they were deprecated in Kubernetes because they were causing memory leaks: kubernetes/kubernetes#74636

the method in controller-runtime was later removed in kubernetes-sigs/controller-runtime#1946, which doesn't affect KubeVirt yet

Reflector metrics were removed in Kubernetes due to them causing memory leaks: kubernetes/kubernetes#74636 Signed-off-by: João Vilaça <jvilaca@redhat.com>

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca · 2024-04-08T14:52:18Z

/test pull-kubevirt-unit-test-arm64
/test pull-kubevirt-e2e-k8s-1.29-sig-storage

sradco · 2024-04-08T15:06:51Z

/lgtm

kubevirt-commenter-bot · 2024-04-08T15:06:56Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

machadovilaca · 2024-04-08T15:56:53Z

/unhold

kubevirt-bot · 2024-04-08T21:08:34Z

@machadovilaca: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.29-sig-compute	`58fb25a`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.29-sig-compute`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kubevirt-commenter-bot · 2024-04-09T00:57:44Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. area/monitoring kind/build-change Categorizes PRs as related to changing build files of virt-* components size/XL labels Feb 7, 2024

kubevirt-bot requested review from jean-edouard and rthallisey February 7, 2024 19:16

kubevirt-bot requested a review from enp0s3 February 7, 2024 19:16

machadovilaca force-pushed the refactor-client-metrics branch 2 times, most recently from b40b83f to 8615658 Compare February 8, 2024 15:38

machadovilaca force-pushed the refactor-client-metrics branch from 8615658 to 8b2440d Compare February 14, 2024 11:19

machadovilaca force-pushed the refactor-client-metrics branch from 8b2440d to 051bcff Compare February 14, 2024 14:24

enp0s3 reviewed Feb 15, 2024

View reviewed changes

machadovilaca force-pushed the refactor-client-metrics branch from 051bcff to 69c35a5 Compare February 20, 2024 17:08

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2024

machadovilaca force-pushed the refactor-client-metrics branch from 69c35a5 to f375f84 Compare February 27, 2024 18:53

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2024

assafad mentioned this pull request Mar 7, 2024

Apply monitoring linters #11454

Draft

8 tasks

machadovilaca force-pushed the refactor-client-metrics branch from f375f84 to 55031cb Compare March 8, 2024 17:24

kubevirt-bot added sig/buildsystem Denotes an issue or PR that relates to changes in the build system. size/XXL sig/scale and removed size/XL labels Mar 8, 2024

machadovilaca changed the title ~~Refactor client metrics~~ Refactor common component metrics Mar 8, 2024

kubevirt-bot assigned sradco Apr 2, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2024

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 5, 2024

Refactor client metrics

cea3027

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca force-pushed the refactor-client-metrics branch from 55031cb to 8552eb1 Compare April 8, 2024 12:45

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024

machadovilaca added 2 commits April 8, 2024 13:50

Remove reflector metrics

b66f627

Reflector metrics were removed in Kubernetes due to them causing memory leaks: kubernetes/kubernetes#74636 Signed-off-by: João Vilaça <jvilaca@redhat.com>

Refactor workqueue metrics

58fb25a

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca force-pushed the refactor-client-metrics branch from 8552eb1 to 58fb25a Compare April 8, 2024 12:50

avlitman mentioned this pull request Apr 8, 2024

Refactor metrics doc generator #11467

Closed

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2024

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2024

kubevirt-bot merged commit a2e118f into kubevirt:main Apr 9, 2024
39 of 40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor common component metrics #11184

Refactor common component metrics #11184

machadovilaca commented Feb 7, 2024 •

edited

machadovilaca commented Feb 7, 2024

machadovilaca commented Feb 12, 2024

machadovilaca commented Feb 14, 2024

machadovilaca commented Feb 14, 2024

enp0s3 left a comment

enp0s3 Feb 15, 2024

machadovilaca Feb 19, 2024

enp0s3 Feb 15, 2024

machadovilaca Feb 19, 2024

avlitman Mar 24, 2024

machadovilaca Mar 25, 2024

sradco Mar 31, 2024

machadovilaca Apr 1, 2024

machadovilaca commented Feb 20, 2024

machadovilaca commented Feb 20, 2024

acardace commented Apr 3, 2024

kubevirt-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 4, 2024

kubevirt-commenter-bot commented Apr 4, 2024

kubevirt-commenter-bot commented Apr 5, 2024

kubevirt-commenter-bot commented Apr 5, 2024

kubevirt-commenter-bot commented Apr 5, 2024

fossedihelm commented Apr 5, 2024

machadovilaca commented Apr 8, 2024 •

edited

machadovilaca commented Apr 8, 2024

sradco commented Apr 8, 2024

kubevirt-commenter-bot commented Apr 8, 2024

machadovilaca commented Apr 8, 2024

kubevirt-bot commented Apr 8, 2024

kubevirt-commenter-bot commented Apr 9, 2024

Refactor common component metrics #11184

Refactor common component metrics #11184

Conversation

machadovilaca commented Feb 7, 2024 • edited

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

machadovilaca commented Feb 7, 2024

machadovilaca commented Feb 12, 2024

machadovilaca commented Feb 14, 2024

machadovilaca commented Feb 14, 2024

enp0s3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machadovilaca commented Feb 20, 2024

machadovilaca commented Feb 20, 2024

acardace commented Apr 3, 2024

kubevirt-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 3, 2024

kubevirt-commenter-bot commented Apr 4, 2024

kubevirt-commenter-bot commented Apr 4, 2024

kubevirt-commenter-bot commented Apr 5, 2024

kubevirt-commenter-bot commented Apr 5, 2024

kubevirt-commenter-bot commented Apr 5, 2024

fossedihelm commented Apr 5, 2024

machadovilaca commented Apr 8, 2024 • edited

machadovilaca commented Apr 8, 2024

sradco commented Apr 8, 2024

kubevirt-commenter-bot commented Apr 8, 2024

machadovilaca commented Apr 8, 2024

kubevirt-bot commented Apr 8, 2024

kubevirt-commenter-bot commented Apr 9, 2024

machadovilaca commented Feb 7, 2024 •

edited

machadovilaca commented Apr 8, 2024 •

edited