[CI] Add metrics name linter #2774

avlitman · 2023-06-26T19:28:05Z

This linter will lint the metrics in docs/metrics.md file. In order to run the linter:
1.make lint-metrics

What this PR does / why we need it:
jira-ticket: https://issues.redhat.com/browse/CNV-29824

Reviewer notes:
Linter output:

clone_progress: counter metrics should have "_total" or "_timestamp_seconds" suffix
clone_progress: name need to start with 'kubevirt_'
kubevirt_cdi_incomplete_storageprofiles_total: non-counter metrics should not have "_total" suffix
kubevirt_cdi_operator_up_total: non-counter metrics should not have "_total" suffix
make: *** [Makefile:213: lint-metrics] Error 1

note that we ignore all current errors until the metrics names will be fixed in:#2773

Release note:

Add a metrics name linter. In order to monitor metrics naming and make sure they aligned with the Prometheus metrics naming best practices.

kubevirt-bot · 2023-06-26T19:28:15Z

Hi @avlitman. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

avlitman · 2023-06-26T19:33:05Z

Hi @arnongilboa @maya-r, when running make lint-metrics it pass, but when running make test-lint it fail:

...
Starting rsyncd

Rsyncing /home/alitman/Projects/github.com/kubevirt.io/containerized-data-importer to container
Starting bazel server
go version go1.19.5 linux/amd64
CDI_CRI: , CDI_CONTAINER_BUILDCMD: buildah
FAIL: Format errors found in the following files:
tools/prom-metrics-collector/metrics_collector.go
running golint on directory: staging/src/kubevirt.io/containerized-data-importer-api
running golint on directory: pkg
running golint on directory: cmd
running golint on directory: tests
running golint on directory: tools
+ GOLANGCI_VERSION=v1.52.2
+ go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.52.2
+ golangci-lint run --timeout=16m ./...
make: *** [Makefile:84: test-lint] Error 1

Not sure why, any ideas?

awels · 2023-06-26T19:53:00Z

This is the problem file tools/prom-metrics-collector/metrics_collector.go run gofmt -w tools/prom-metrics-collector/metrics_collector.go

avlitman · 2023-06-27T07:01:08Z

gofmt -w tools/prom-metrics-collector/metrics_collector.go

@awels thanks!
There is no output unfortunately: [alitman@fedora containerized-data-importer]$ gofmt -w tools/prom-metrics-collector/metrics_collector.go

awels · 2023-06-27T12:10:43Z

The -w tells it to write the output to the file, if you look at git status you will see the file is modified, git diff should show you what changed.

avlitman · 2023-06-27T12:17:24Z

The -w tells it to write the output to the file, if you look at git status you will see the file is modified, git diff should show you what changed.

@awels I see only changes in vendor dir no other dirs or a new file under cdi.. and when running without the -w it just show me the file.
So I can't tell what is the issue (by the way this file has no issues in kubevirt or hco operators, so maybe it's something broken?)

awels · 2023-06-27T12:46:16Z

So gofmt takes an input file and formats it according to the standard golang formatting rules. if you add -w it will write the changes directly to the file, if you don't use it, it should display the diff on stdout.

Did you add tools/prom-metrics-collector/metrics_collector.go to the commit? Even if not, after running gofmt -w tools/prom-metrics-collector/metrics_collector.go running make test-lint should now pass. It is simply running gofmt and checking if there is any output.

avlitman · 2023-06-27T13:21:42Z

So gofmt takes an input file and formats it according to the standard golang formatting rules. if you add -w it will write the changes directly to the file, if you don't use it, it should display the diff on stdout.

Did you add tools/prom-metrics-collector/metrics_collector.go to the commit? Even if not, after running gofmt -w tools/prom-metrics-collector/metrics_collector.go running make test-lint should now pass. It is simply running gofmt and checking if there is any output.

Maybe I'm doing something wrong but it didn't pass:

[alitman@fedora containerized-data-importer]$ gofmt -w tools/prom-metrics-collector/metrics_collector.go
[alitman@fedora containerized-data-importer]$ make test-lint
./hack/ci/prom_metric_linter.sh --operator-name="kubevirt" --sub-operator-name="cdi"
warning: GOPATH set to GOROOT (/usr/local/go) has no effect
CDI_CRI: docker, CDI_CONTAINER_BUILDCMD: docker
./hack/build/bazel-docker.sh "./hack/build/run-lint-checks.sh"
CDI_CRI: docker, CDI_CONTAINER_BUILDCMD: docker
Making sure output directory exists...
go version go1.19.5 linux/amd64
go version go1.19.5 linux/amd64
Starting rsyncd

Rsyncing /home/alitman/Projects/github.com/kubevirt.io/containerized-data-importer to container
aaff9f139aac1ff37d34ab15e5eca4db6e8fa140f5775c8cd030d11b3569e04a
Starting bazel server
go version go1.19.5 linux/amd64
CDI_CRI: , CDI_CONTAINER_BUILDCMD: buildah
FAIL: Format errors found in the following files:
tools/prom-metrics-collector/metrics_collector.go
running golint on directory: staging/src/kubevirt.io/containerized-data-importer-api
running golint on directory: pkg
running golint on directory: cmd
running golint on directory: tests
running golint on directory: tools
+ GOLANGCI_VERSION=v1.52.2
+ go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.52.2
+ golangci-lint run --timeout=16m ./...
make: *** [Makefile:84: test-lint] Error 1

@awels thanks for you patience!

awels · 2023-06-28T18:00:00Z

/ok-to-test

awels · 2023-06-28T18:08:35Z

kubevirt/project-infra#2857 should fix the current linter error we are seeing in CI

awels · 2023-06-28T18:20:18Z

Odd gofmt -w doesn't seem to fix it, but I managed to get it to tell me what it thought was wrong

diff --git a/tools/prom-metrics-collector/metrics_collector.go b/tools/prom-metrics-collector/metrics_collector.go
index 22f3c071f..83bda59eb 100644
--- a/tools/prom-metrics-collector/metrics_collector.go
+++ b/tools/prom-metrics-collector/metrics_collector.go
@@ -30,9 +30,9 @@ import (
 // open issue:https://github.com/kubevirt/containerized-data-importer/issues/2773
 // Do not add metrics to this list!
 var excludedMetrics = map[string]struct{}{
-       "clone_progress":                                struct{}{},
-       "kubevirt_cdi_operator_up_total":                struct{}{},
-       "kubevirt_cdi_incomplete_storageprofiles_total": struct{}{},
+       "clone_progress":                                {},
+       "kubevirt_cdi_operator_up_total":                {},
+       "kubevirt_cdi_incomplete_storageprofiles_total": {},
 }

awels · 2023-06-28T19:30:03Z

/test pull-cdi-linter

Signed-off-by: Aviv Litman <alitman@redhat.com>

avlitman · 2023-06-29T06:41:31Z

Odd gofmt -w doesn't seem to fix it, but I managed to get it to tell me what it thought was wrong

diff --git a/tools/prom-metrics-collector/metrics_collector.go b/tools/prom-metrics-collector/metrics_collector.go
index 22f3c071f..83bda59eb 100644
--- a/tools/prom-metrics-collector/metrics_collector.go
+++ b/tools/prom-metrics-collector/metrics_collector.go
@@ -30,9 +30,9 @@ import (
 // open issue:https://github.com/kubevirt/containerized-data-importer/issues/2773
 // Do not add metrics to this list!
 var excludedMetrics = map[string]struct{}{
-       "clone_progress":                                struct{}{},
-       "kubevirt_cdi_operator_up_total":                struct{}{},
-       "kubevirt_cdi_incomplete_storageprofiles_total": struct{}{},
+       "clone_progress":                                {},
+       "kubevirt_cdi_operator_up_total":                {},
+       "kubevirt_cdi_incomplete_storageprofiles_total": {},
 }

Fixed.
Thank you!

avlitman · 2023-06-29T06:42:29Z

/ok-to-test

awels · 2023-06-29T12:13:33Z

/lgtm
@akalenyu can you review?

akalenyu · 2023-07-02T07:34:35Z

/approve

kubevirt-bot · 2023-07-02T07:35:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

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

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

akalenyu · 2023-07-02T08:03:35Z

/test pull-containerized-data-importer-non-csi-hpp

avlitman · 2023-07-02T11:14:41Z

/retest

akalenyu · 2023-07-02T14:01:25Z

/test pull-containerized-data-importer-e2e-ceph

Signed-off-by: Aviv Litman <alitman@redhat.com>

* dataimportcron: Pass dynamic credential support label (#2760) * dataimportcron: code change: Use better matchers in tests Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> * dataimportcron: Pass dynamic credential support label The label is passed from DataImportCron to DataVolume and DataSource. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> --------- Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> * Add DataImportCron snapshot sources docs (#2747) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * add akalenyu as approver, some others as reviewers (#2766) Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Run `make rpm-deps` (#2741) * Run make rpm-deps Signed-off-by: Maya Rashish <mrashish@redhat.com> * Avoid overlayfs error message by using vfs driver Signed-off-by: Maya Rashish <mrashish@redhat.com> --------- Signed-off-by: Maya Rashish <mrashish@redhat.com> * Fix Destructive test lane failure - missing pod following recreate of CDI (#2744) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * [WIP] Handle nil ptr in dataimportcron controller (#2769) Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Revert some gomega error checking that produce confusing output (#2772) One of these tests flakes, but the error is hard to debug because gomega will yell about `Unexpected non-nil/non-zero argument at index 0` Instead of showing the error. Apparently this is intended: https://github.com/onsi/gomega/pull/480/files#diff-e696deff1a5be83ad03053b772926cb325cede3b33222fa76c2bb1fcf2efd809R186-R190 Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Run bazelisk run //robots/cmd/uploader:uploader -- -workspace /home/prow/go/src/github.com/kubevirt/project-infra/../containerized-data-importer/WORKSPACE -dry-run=false (#2770) Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> * [CI] Add metrics name linter (#2774) Signed-off-by: Aviv Litman <alitman@redhat.com> --------- Signed-off-by: Andrej Krejcir <akrejcir@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Michael Henriksen <mhenriks@redhat.com> Signed-off-by: Maya Rashish <mrashish@redhat.com> Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> Signed-off-by: Aviv Litman <alitman@redhat.com> Co-authored-by: Andrej Krejcir <akrejcir@gmail.com> Co-authored-by: Michael Henriksen <mhenriks@redhat.com> Co-authored-by: Maya Rashish <mrashish@redhat.com> Co-authored-by: kubevirt-bot <kubevirtbot@redhat.com> Co-authored-by: Aviv Litman <64130977+avlitman@users.noreply.github.com>

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 26, 2023

kubevirt-bot requested review from arnongilboa and maya-r June 26, 2023 19:28

kubevirt-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 26, 2023

kubevirt-bot added the size/L label Jun 26, 2023

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 28, 2023

[CI] Add metrics name linter

d659a08

Signed-off-by: Aviv Litman <alitman@redhat.com>

avlitman force-pushed the add-metrics-linter branch from 647445b to d659a08 Compare June 29, 2023 06:40

avlitman mentioned this pull request Jun 29, 2023

Metrics names failed promlint linter #2773

Closed

kubevirt-bot assigned awels Jun 29, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2023

kubevirt-bot assigned akalenyu Jul 2, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2023

kubevirt-bot merged commit 9e37493 into kubevirt:main Jul 2, 2023
1 check passed

akalenyu pushed a commit to akalenyu/containerized-data-importer that referenced this pull request Jul 3, 2023

[CI] Add metrics name linter (kubevirt#2774)

cc1a3ed

Signed-off-by: Aviv Litman <alitman@redhat.com>

avlitman mentioned this pull request Jul 13, 2023

Apply metrics name linter kubevirt/ssp-operator#621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Add metrics name linter #2774

[CI] Add metrics name linter #2774

avlitman commented Jun 26, 2023 •

edited

Loading

kubevirt-bot commented Jun 26, 2023

avlitman commented Jun 26, 2023 •

edited

Loading

awels commented Jun 26, 2023

avlitman commented Jun 27, 2023 •

edited

Loading

awels commented Jun 27, 2023

avlitman commented Jun 27, 2023 •

edited

Loading

awels commented Jun 27, 2023

avlitman commented Jun 27, 2023

awels commented Jun 28, 2023

awels commented Jun 28, 2023

awels commented Jun 28, 2023

awels commented Jun 28, 2023

avlitman commented Jun 29, 2023 •

edited

Loading

avlitman commented Jun 29, 2023

awels commented Jun 29, 2023

akalenyu commented Jul 2, 2023

kubevirt-bot commented Jul 2, 2023

akalenyu commented Jul 2, 2023

avlitman commented Jul 2, 2023

akalenyu commented Jul 2, 2023

[CI] Add metrics name linter #2774

[CI] Add metrics name linter #2774

Conversation

avlitman commented Jun 26, 2023 • edited Loading

kubevirt-bot commented Jun 26, 2023

avlitman commented Jun 26, 2023 • edited Loading

awels commented Jun 26, 2023

avlitman commented Jun 27, 2023 • edited Loading

awels commented Jun 27, 2023

avlitman commented Jun 27, 2023 • edited Loading

awels commented Jun 27, 2023

avlitman commented Jun 27, 2023

awels commented Jun 28, 2023

awels commented Jun 28, 2023

awels commented Jun 28, 2023

awels commented Jun 28, 2023

avlitman commented Jun 29, 2023 • edited Loading

avlitman commented Jun 29, 2023

awels commented Jun 29, 2023

akalenyu commented Jul 2, 2023

kubevirt-bot commented Jul 2, 2023

akalenyu commented Jul 2, 2023

avlitman commented Jul 2, 2023

akalenyu commented Jul 2, 2023

avlitman commented Jun 26, 2023 •

edited

Loading

avlitman commented Jun 26, 2023 •

edited

Loading

avlitman commented Jun 27, 2023 •

edited

Loading

avlitman commented Jun 27, 2023 •

edited

Loading

avlitman commented Jun 29, 2023 •

edited

Loading