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

Add RHACM Openshift Virtualization Dashboards #1603

Closed
wants to merge 2 commits into from

Conversation

leoaaraujo
Copy link

Added Grafana Dashboards for Openshift Virtualization managed by ACM.

"targets": [
{
"exemplar": true,
"expr": "csv_succeeded{name=~\".*hyperconverged.*\", cluster=~\"$cluster\"}",
Copy link
Contributor

Choose a reason for hiding this comment

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

We can replace with acm_managed_cluster_labels since the OpenShift Virtualization version aligns with OpenShift version.

Copy link
Author

Choose a reason for hiding this comment

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

Shirly, the major version is aligned, but the minor version will not always be the same, so we chose to use csv_succeed to present the OCP Virt version.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 In that case I suggest to limit the metric collection to csv_succeeded{name=~".*hyperconverged.*"} instead of just csv_succeeded if its not needed by other dashboards. WDTY?

Copy link
Author

Choose a reason for hiding this comment

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

I understood that the suggestion is not directly related to the dashboard, but rather the custom-metrics-allowlist configmap, the csv_succeed metric already collected by default and is not part of the custom metrics configmap.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I forgot this was collected also for other dashboards.

@philipgough
Copy link
Contributor

/test test-e2e

Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

/lgtm

@sradco
Copy link
Contributor

sradco commented Sep 11, 2024

We are still working making changes to the dashboards.

/hold

Copy link

openshift-ci bot commented Sep 11, 2024

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

  • ecf19a7 namespace added to the metrics configmap & dashboards improvements

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-sigs/prow repository. I understand the commands that are listed here.

Copy link

Copy link
Contributor

@moadz moadz left a comment

Choose a reason for hiding this comment

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

LGTM pending we can test these in a live environment - one note is all the dashboards need a unique UUID as per this PR
https://github.com/stolostron/multicluster-observability-operator/pull/1566/files

So that when they link to themselves it's a fixed URL :)

Copy link

openshift-ci bot commented Sep 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leoaaraujo, moadz, philipgough

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

@sradco
Copy link
Contributor

sradco commented Sep 17, 2024

/hold
This PR was replaced by #1613

@moadz moadz closed this Oct 15, 2024
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.

5 participants