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

Enable ingestion of the exposed metrics by openshift-montoring #410

Merged

Conversation

shubham-pampattiwar
Copy link
Member

@shubham-pampattiwar shubham-pampattiwar commented Oct 21, 2021

This PR enable the ingestion of the velero metrics by openshift monitoring. We are deploying the service and serviceMonitor required for the metrics to be scraped by prometheus. We propose the following metrics for the monitoring and telemetry purposes:

"velero_backup_total"
"velero_restore_total"

@openshift-ci
Copy link

openshift-ci bot commented Oct 21, 2021

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

Merging #410 (c99f511) into master (c1c6b2d) will increase coverage by 0.60%.
The diff coverage is 43.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   37.51%   38.11%   +0.60%     
==========================================
  Files          12       13       +1     
  Lines        2287     2571     +284     
==========================================
+ Hits          858      980     +122     
- Misses       1358     1514     +156     
- Partials       71       77       +6     
Impacted Files Coverage Δ
controllers/dpa_controller.go 0.00% <0.00%> (ø)
controllers/monitor.go 41.63% <41.63%> (ø)
controllers/velero.go 38.81% <64.00%> (+1.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0adb97b...c99f511. Read the comment docs.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021
@shubham-pampattiwar shubham-pampattiwar marked this pull request as ready for review December 1, 2021 22:29
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2021
@shubham-pampattiwar shubham-pampattiwar self-assigned this Dec 1, 2021
@kaovilai
Copy link
Member

kaovilai commented Dec 3, 2021

/retest

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2021
- patch
- watch
- apiGroups:
- rbac.authorization.k8s.io
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry about adding these permissions to the operator, is there any other way of achieving this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@shawn-hurley we need these permissions so that the operator creates the appropriate role/rolebindings needed for the prometheus operator to scrape the metrics from our ns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, isn't that already done by OLM when installing if you have the correct annotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

@shawn-hurley PTAL I have removed the prometheus role/rolebinding creation by OADP operator.

@smarterclayton
Copy link

Is this cardinality 13 or higher for series count planned to send to telemetry?

@shawn-hurley
Copy link
Contributor

@smarterclayton is the cardinality of 13 or lower for each metric, or in total for all metrics?

@shawn-hurley
Copy link
Contributor

@smarterclayton never mind you said series, not metrics sorry

IIUC this means that that we can only send 13 series to monitoring. The key distinction here is going to be that when a label is added to the data, you increase the cardinality by 1.

@shubham-pampattiwar
Copy link
Member Author

Is this cardinality 13 or higher for series count planned to send to telemetry?

@smarterclayton After re-evaluating our telemetry needs, we have decided to use only two metrics to be ingested by openshift-monitoring: velero_backup_total and velero_restore_total . Both these metrics do not have any kind of label, so I believe the cardinality would be 2.

@dymurray
Copy link
Member

/retest

1 similar comment
@dymurray
Copy link
Member

/retest

@dymurray
Copy link
Member

dymurray commented Jan 7, 2022

/retest

@openshift-ci
Copy link

openshift-ci bot commented Jan 7, 2022

@shubham-pampattiwar: 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
ci/prow/operator-e2e 305bb6d link true /test operator-e2e

Full PR test history. Your PR dashboard.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants