Progress metrics refactor and rename #3254

arnongilboa · 2024-05-12T14:49:03Z

What this PR does / why we need it:
clone_progress, openstack_populator_progress and ovirt_progress metrics are not in the monitoring package. These metrics are not with correct metrics names, based on the kubevirt and Prometheus metrics naming conventions, they are not documented and are not located under /pkg/monitoring. After the code refactoring we should not have Prometheus metrics in other places in the code, other than the /monitoring/metrics package, and metrics should be registered using operator-observability package.

In addition, we aligned the progress metric names with the metric naming linter rules: kubevirt_cdi_clone_progress_total, kubevirt_cdi_openstack_populator_progress_total, kubevirt_cdi_ovirt_progress_total.

Which issue(s) this PR fixes:
Fixes # https://issues.redhat.com/browse/CNV-36845

Special notes for your reviewer:

Release note:

Align progress metrics names with kubevirt and Prometheus metrics naming conventions

coveralls · 2024-05-12T14:55:59Z

coverage: 17.259% (+0.02%) from 17.24%
when pulling adb1b1d on arnongilboa:clone_progress_metric_refactor
into d2f3903 on kubevirt:main.

machadovilaca · 2024-05-13T10:27:05Z

pkg/util/prometheus/prometheus.go

@@ -66,12 +64,12 @@ func (r *ProgressReader) updateProgress() bool {
 			currentProgress = float64(r.Current) / float64(r.total) * 100.0
 		}
 		metric := &dto.Metric{}


we could also move this very specific Prometheus metric impl to the monitoring package, and make WriteCloneProgress return the value, which we would use later instead of *metric.Counter.Value

machadovilaca · 2024-05-13T10:29:40Z

pkg/monitoring/metrics/cdi-cloner/clone_metrics.go

+func InitCloneProgressCounterVec() {
+	cloneProgress = newCloneProgressCounterVec()
+}


I think we can avoid this, and inline the metric declaration, if in the test you call SetupMetrics

machadovilaca · 2024-05-13T10:35:32Z

pkg/monitoring/metrics/openstack-populator/populator_metrics.go

+			//Subsystem: "volume_populators",
+			Name: "openstack_volume_populator",


with

Subsystem: "volume_populators", Name: "openstack_volume_populator",

the name was: "volume_populators_openstack_volume_populator", here we should have
https://github.com/prometheus/client_golang/blob/6e3f4b1091875216850a486b1c2eb0e5ea852f98/prometheus/metric.go#L69

Suggested change

//Subsystem: "volume_populators",

Name: "openstack_volume_populator",

Name: "volume_populators_openstack_volume_populator",

If we are generating the metric it should have kubevirt_cdi_ prefix

machadovilaca · 2024-05-13T10:37:49Z

pkg/util/prometheus/prometheus_test.go

 		metric := &dto.Metric{}
-		Expect(progress.WithLabelValues(ownerUID).Write(metric)).To(Succeed())
+		Expect(metrics.WriteCloneProgress(ownerUID, metric)).To(Succeed())
 		Expect(*metric.Counter.Value).To(Equal(float64(0)))


instead of using &dto.Metric{}, create a method to get the value of a metric in pkg/monitoring, such as https://github.com/kubevirt/kubevirt/blob/e6207f255d85c24567f1d28b5a6f232279ae68ff/pkg/monitoring/metrics/virt-controller/vmi_metrics.go#L43

machadovilaca · 2024-05-13T10:43:24Z

pkg/monitoring/metrics/cdi-cloner/clone_metrics.go

+}
+
+// WriteCloneProgress writes the cloneProgress metric into a metric protocol buffer
+func WriteCloneProgress(labelValue string, metric *ioprometheusclient.Metric) error {


we should avoid this and only use add/set operations, this file should abstract Prometheus to the outside

sradco · 2024-05-15T21:08:12Z

pkg/monitoring/metrics/ovirt-populator/populator-metrics.go

+
+	populatorProgress = operatormetrics.NewCounterVec(
+		operatormetrics.MetricOpts{
+			Name: "ovirt_progress",


Same. The metric requires kubevirt_cdi_ prefix

Also missing _total suffix, since it's a counter.

@bennyz wdyt?

assafad · 2024-05-16T07:06:18Z

@arnongilboa We have a metric name linter, can you please add the new metrics in
https://github.com/kubevirt/containerized-data-importer/blob/main/tools/prom-metrics-collector/metrics_json_generator.go? You can then use it locally with make lint-metrics and see what names you need to fix.
@machadovilaca @avlitman @sradco FYI, when new metrics groups are added, the developer needs to add the new group to the metrics names linter, or else CI won't catch it.

The clone_progress metric is not in the monitoring package. The metric is with incorrect name, based on the kubevirt and Prometheus metrics naming conventions. It's not documented and not located under /pkg/monitoring. After the code refactoring we should not have Prometheus metrics in other places in the code, other than the /monitoring/metrics package, and metrics should be registered using operator-observability package. Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

Also add the metrics to the doc and json generation tools. Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

awels

Will wait for the metrics folks to give their blessing, but code wise looks good to me.

awels · 2024-05-16T14:27:18Z

/test pull-cdi-goveralls

machadovilaca

nit: I think we could avoid calling SetupMetrics in the init functions, but let's not block this because of that

machadovilaca · 2024-05-16T15:12:28Z

/lgtm

awels · 2024-05-16T16:51:01Z

/approve

kubevirt-bot · 2024-05-16T16:51:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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~~ [awels]

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

sradco · 2024-05-16T21:27:05Z

doc/metrics.md

 ### kubevirt_cdi_operator_up
 CDI operator status. Type: Gauge.

+### kubevirt_cdi_ovirt_progress_total


@arnongilboa shouldn't the metric name include populator like it is for openstack?

@arnongilboa can you please update the metrics description to be clearer? They are the same for both the openstack and ovirt metrics

@bennyz please consider doing it in a follow-up PR

Sure. I'm OK with that.

arnongilboa · 2024-05-17T10:15:47Z

/retest

sradco · 2024-05-17T10:29:19Z

/lgtm

kubevirt_cdi_ovirt_progress_total metric name and description would be updated in a follow-up PR.
This PR is urgent to unblock the metrics code refactoring linter PR.

Manual backport of kubevirt#3254 clone_progress, openstack_populator_progress and ovirt_progress metrics are not in the monitoring package. These metrics are not with correct metrics names, based on the kubevirt and Prometheus metrics naming conventions, they are not documented and are not located under /pkg/monitoring. After the code refactoring we should not have Prometheus metrics in other places in the code, other than the /monitoring/metrics package, and metrics should be registered using operator-observability package. In addition, we aligned the progress metric names with the metric naming linter rules: kubevirt_cdi_clone_progress_total kubevirt_cdi_openstack_populator_progress_total kubevirt_cdi_ovirt_progress_total Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

Use default metric registration. We shouldn't use the controller-runtime registration as we have no controller here and it will not register the metric correctly. Fix the metric parsing to match its new name. Otherwise the DV progress will not be updated until its 100%. Regression introduced in kubevirt#3254 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

Use default metric registration. We shouldn't use the controller-runtime registration as we have no controller here and it will not register the metric correctly. Fix the metric parsing to match its new name. Otherwise the DV progress will not be updated until its 100%. Regression introduced in kubevirt#3254 Signed-off-by: Arnon Gilboa <agilboa@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

Use default metric registration. We shouldn't use the controller-runtime registration as we have no controller here and it will not register the metric correctly. Fix the metric parsing to match its new name. Otherwise the DV progress will not be updated until its 100%. Regression introduced in kubevirt#3254 Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

* Fix progress metric registration and parsing Use default metric registration. We shouldn't use the controller-runtime registration as we have no controller here and it will not register the metric correctly. Fix the metric parsing to match its new name. Otherwise the DV progress will not be updated until its 100%. Regression introduced in #3254 Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Add kubevirt_cdi_import_progress_total metric Use it in the importer instead of kubevirt_cdi_clone_progress_total and fix metric parsing accordingly. Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Move ProgressFromClaim to host-clone Nobody else is using it. Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Add ProgressMetric interface ProgressReader can now work with either import or clone progress metric. FIXME: consider removing the direct Add/Get and use only via interface. Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Refactor ProgressMetric interface Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Refactor progress parsing Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Refer metric names from the metrics package Signed-off-by: Arnon Gilboa <agilboa@redhat.com> --------- Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

* [release-v1.59] Progress metrics refactor and rename Manual backport of #3254 clone_progress, openstack_populator_progress and ovirt_progress metrics are not in the monitoring package. These metrics are not with correct metrics names, based on the kubevirt and Prometheus metrics naming conventions, they are not documented and are not located under /pkg/monitoring. After the code refactoring we should not have Prometheus metrics in other places in the code, other than the /monitoring/metrics package, and metrics should be registered using operator-observability package. In addition, we aligned the progress metric names with the metric naming linter rules: kubevirt_cdi_clone_progress_total kubevirt_cdi_openstack_populator_progress_total kubevirt_cdi_ovirt_progress_total Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * [release-v1.59] Fix progress metric registration and parsing Manual backport of #3292 Use default metric registration. We shouldn't use the controller-runtime registration as we have no controller here and it will not register the metric correctly. Add kubevirt_cdi_import_progress_total metric and use it in the importer instead of kubevirt_cdi_clone_progress_total. Fix the metric parsing to match their new names. Otherwise the DV progress will not be updated until its 100%. Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * [release-v1.59] Fix progress reporting for http imports Backport of #3293 * tests: fix regex in import progress test This test currently passes since the regex will always catch 100%, hiding a bug we have in progress reporting. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Fix actual progress reporting for url->scratch Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Hold on with reporting 100% until pod exits Otherwise, we'll be reporting 100% prior to convert/resize/... being done Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * tests: fix a test that was adapted to no progress reporting this tests cares about some progress being made so N/A isn't the right choice Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Arnon Gilboa <agilboa@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Co-authored-by: Alex Kalenyuk <akalenyu@redhat.com>

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. labels May 12, 2024

kubevirt-bot requested review from aglitke and ShellyKa13 May 12, 2024 14:49

kubevirt-bot added the size/L label May 12, 2024

arnongilboa changed the title ~~Clone progress metric refactor~~ clone_progress and openstack_volume_populator metrics refactor May 13, 2024

arnongilboa force-pushed the clone_progress_metric_refactor branch 2 times, most recently from 3a816be to ded9c22 Compare May 13, 2024 09:44

machadovilaca suggested changes May 13, 2024

View reviewed changes

kubevirt-bot assigned machadovilaca May 13, 2024

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

arnongilboa force-pushed the clone_progress_metric_refactor branch from ded9c22 to 0245f42 Compare May 15, 2024 19:59

kubevirt-bot added size/XL and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L labels May 15, 2024

arnongilboa changed the title ~~clone_progress and openstack_volume_populator metrics refactor~~ Progress metrics refactor May 15, 2024

sradco reviewed May 15, 2024

View reviewed changes

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

arnongilboa added 4 commits May 16, 2024 11:20

openstack_populator_progress metric refactor

0e1a7a3

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

clone_progress metric refactor CR fixes

4ac310a

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

ovirt_progress metric refactor

d189bfe

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

arnongilboa force-pushed the clone_progress_metric_refactor branch from 0245f42 to d189bfe Compare May 16, 2024 08:28

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

arnongilboa changed the title ~~Progress metrics refactor~~ Progress metrics refactor and rename May 16, 2024

arnongilboa force-pushed the clone_progress_metric_refactor branch 2 times, most recently from d72e060 to 2490c89 Compare May 16, 2024 11:45

Align progress metrics names with linter rules

4161f1c

Also add the metrics to the doc and json generation tools. Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

awels approved these changes May 16, 2024

View reviewed changes

kubevirt-bot assigned awels May 16, 2024

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

machadovilaca reviewed May 16, 2024

View reviewed changes

assafad mentioned this pull request May 16, 2024

Apply monitoring linters #3225

Merged

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

kubevirt-bot assigned sradco May 16, 2024

sradco reviewed May 16, 2024

View reviewed changes

kubevirt-bot merged commit a5cedb1 into kubevirt:main May 17, 2024
19 checks passed

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 18, 2024

arnongilboa mentioned this pull request May 18, 2024

[release-v1.59] Progress metrics refactor and rename #3270

Merged

arnongilboa mentioned this pull request May 27, 2024

Fix progress metric registration and parsing #3292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Progress metrics refactor and rename #3254

Progress metrics refactor and rename #3254

arnongilboa commented May 12, 2024 •

edited

Loading

coveralls commented May 12, 2024 •

edited

Loading

machadovilaca May 13, 2024

machadovilaca May 13, 2024

machadovilaca May 13, 2024

sradco May 15, 2024

machadovilaca May 13, 2024

machadovilaca May 13, 2024

sradco May 15, 2024

assafad May 16, 2024 •

edited

Loading

arnongilboa May 16, 2024

arnongilboa May 16, 2024

assafad commented May 16, 2024 •

edited

Loading

awels left a comment

awels commented May 16, 2024

machadovilaca left a comment

machadovilaca commented May 16, 2024

awels commented May 16, 2024

kubevirt-bot commented May 16, 2024

sradco May 16, 2024

sradco May 16, 2024 •

edited

Loading

arnongilboa May 17, 2024

sradco May 17, 2024

arnongilboa commented May 17, 2024

sradco commented May 17, 2024

		//Subsystem: "volume_populators",
		Name: "openstack_volume_populator",

	//Subsystem: "volume_populators",
	Name: "openstack_volume_populator",
	Name: "volume_populators_openstack_volume_populator",

Progress metrics refactor and rename #3254

Progress metrics refactor and rename #3254

Conversation

arnongilboa commented May 12, 2024 • edited Loading

coveralls commented May 12, 2024 • edited Loading

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

assafad May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

assafad commented May 16, 2024 • edited Loading

awels left a comment

Choose a reason for hiding this comment

awels commented May 16, 2024

machadovilaca left a comment

Choose a reason for hiding this comment

machadovilaca commented May 16, 2024

awels commented May 16, 2024

kubevirt-bot commented May 16, 2024

Choose a reason for hiding this comment

sradco May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa commented May 17, 2024

sradco commented May 17, 2024

arnongilboa commented May 12, 2024 •

edited

Loading

coveralls commented May 12, 2024 •

edited

Loading

assafad May 16, 2024 •

edited

Loading

assafad commented May 16, 2024 •

edited

Loading

sradco May 16, 2024 •

edited

Loading