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

feat: Metrics unit tests #848

Merged
merged 4 commits into from
Jan 24, 2024
Merged

feat: Metrics unit tests #848

merged 4 commits into from
Jan 24, 2024

Conversation

akrejcir
Copy link
Collaborator

What this PR does / why we need it:
Added unit tests for prometheus metrics alerts.

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

Release note:

None

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 19, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jan 19, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@github-actions GitHub Actions
Copy link

/cc sradco

@kubevirt-bot kubevirt-bot requested a review from sradco January 19, 2024 16:12
@github-actions GitHub Actions
Copy link

/cc sradco

@akrejcir akrejcir marked this pull request as ready for review January 22, 2024 14:44
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2024
@akrejcir
Copy link
Collaborator Author

/cc @0xFelix @ksimon1 @machadovilaca

@github-actions GitHub Actions
Copy link

/cc sradco

@github-actions GitHub Actions
Copy link

/cc sradco

@github-actions GitHub Actions
Copy link

/cc sradco

@akrejcir
Copy link
Collaborator Author

/retest

1 similar comment
@akrejcir
Copy link
Collaborator Author

/retest

@github-actions GitHub Actions
Copy link

/cc sradco

@github-actions GitHub Actions
Copy link

/cc sradco

0xFelix
0xFelix reviewed Jan 23, 2024
View reviewed changes
hack/metrics-rules-test.sh Outdated Show resolved Hide resolved
pkg/monitoring/rules/rules.go Show resolved Hide resolved
pkg/monitoring/rules/rules.go Outdated Show resolved Hide resolved
tools/test-rules-writer/test_rules_writer.go Outdated Show resolved Hide resolved
pkg/monitoring/rules/rules-tests.yaml Outdated Show resolved Hide resolved
pkg/monitoring/rules/rules-tests.yaml Show resolved Hide resolved
pkg/monitoring/rules/rules-tests.yaml Outdated Show resolved Hide resolved
pkg/monitoring/rules/rules-tests.yaml Outdated Show resolved Hide resolved
pkg/monitoring/rules/rules-tests.yaml
- interval: "1m"
input_series:
- series: 'kubevirt_ssp_template_validator_rejected_total{pod="virt-template-validator-12345"}'
values: '0+1x10 10x120'
Copy link
Member

Choose a reason for hiding this comment

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

0+1x10 10 could work too, IIRC the last value is repeated indefinitely.

Copy link
Collaborator Author

@akrejcir akrejcir Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here the change did not work. I'm not sure why, probably there is some interpolation and 0+1x10 10 evaluates to a different value at 65 min time.

Copy link
Member

Choose a reason for hiding this comment

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

'a+bxn' becomes 'a a+b a+(2b) a+(3b) … a+(n*b)'
Read this as series starts at a, then n further samples incrementing by b.

Weird. Also, if the last value is repeated, then 0+1x10 could work as well. It could be about the increase function in TemplateValidatorRejectedIncreaseQuery.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, probably.

pkg/monitoring/rules/rules-tests.yaml Outdated Show resolved Hide resolved
@akrejcir
feat: Move metrics rules and alerts to separate package
aa9a6fb 
Moved the definitions of alerts and rules
to pkg/monitoring/rules package.

Signed-off-by: Andrej Krejcir <[email protected]>
@github-actions GitHub Actions
Copy link

/cc sradco

@akrejcir
feat: run unit tests for metrics rules
92d8780 
Added metrics unit tests file and a Makefile
target to run the tests.

Signed-off-by: Andrej Krejcir <[email protected]>
@akrejcir
feat: add more tests for metrics alerts
fbf89bd 
Added unit tests for all SSP alerts.

Signed-off-by: Andrej Krejcir <[email protected]>
@akrejcir
chore: metrics: Make a global variable private
36106a0 
Made "recordRulesDescList" variable private.

Signed-off-by: Andrej Krejcir <[email protected]>
@github-actions GitHub Actions
Copy link

/cc sradco

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

18.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@akrejcir
Copy link
Collaborator Author

/retest

jcanocan
jcanocan reviewed Jan 24, 2024
View reviewed changes
pkg/monitoring/rules/rules.go
import (
"fmt"

promv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: newline

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I usually split the import block into 3 parts:

  • standard library
  • imports from other modules defined in go.mod
  • imports from this project

0xFelix
0xFelix reviewed Jan 24, 2024
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2024
@jcanocan
Copy link
Contributor

Thanks @akrejcir!
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2024
@kubevirt-bot kubevirt-bot merged commit 1fbd840 into kubevirt:main Jan 24, 2024
13 checks passed
@akrejcir akrejcir deleted the metrics-unit-tests branch January 24, 2024 14:13
@akrejcir
Copy link
Collaborator Author

/cherry-pick release-v0.19

@kubevirt-bot
Copy link
Contributor

@akrejcir: #848 failed to apply on top of branch "release-v0.19":

Applying: feat: Move metrics rules and alerts to separate package
Using index info to reconstruct a base tree...
M	internal/operands/metrics/resources.go
M	tests/monitoring_test.go
M	tools/metricsdocs/metricsdocs.go
Falling back to patching base and 3-way merge...
Auto-merging tools/metricsdocs/metricsdocs.go
CONFLICT (content): Merge conflict in tools/metricsdocs/metricsdocs.go
Auto-merging tests/monitoring_test.go
Auto-merging internal/operands/metrics/resources.go
CONFLICT (content): Merge conflict in internal/operands/metrics/resources.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 feat: Move metrics rules and alerts to separate package
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.19

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcanocan jcanocan jcanocan left review comments

@0xFelix 0xFelix 0xFelix left review comments

@ksimon1 ksimon1 Awaiting requested review from ksimon1

@sradco sradco Awaiting requested review from sradco

@lyarwood lyarwood Awaiting requested review from lyarwood

@machadovilaca machadovilaca Awaiting requested review from machadovilaca

Assignees

@jcanocan jcanocan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akrejcir @kubevirt-bot @jcanocan @0xFelix