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

[kube-prometheus-stack] Add condition to Alert Manager default dashboard to prevent adding the dashboard when AM is disabled #4707

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Phenix66
Copy link

@Phenix66 Phenix66 commented Jul 9, 2024

What this PR does / why we need it

When AM is disabled, the default Grafana dashboard for AM is still added. Other services such as the Kube proxy, etcd, etc do not add their dashboards when disabled.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes #

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

@GMartinez-Sisti
Copy link
Member

My first thought is that there might be folks deploying this stack without alertmanager enabled but still wanting to get the dashboard for whatever reason. 🤔

@Phenix66
Copy link
Author

Phenix66 commented Jul 9, 2024

My first thought is that there might be folks deploying this stack without alertmanager enabled but still wanting to get the dashboard for whatever reason. 🤔

I had a similar thought and debated implementing a new value similar to .Values.defaultRules but I decided against that since there were already other dashboards that simply relied upon the relevant enabled value.

@GMartinez-Sisti
Copy link
Member

My first thought is that there might be folks deploying this stack without alertmanager enabled but still wanting to get the dashboard for whatever reason. 🤔

I had a similar thought and debated implementing a new value similar to .Values.defaultRules but I decided against that since there were already other dashboards that simply relied upon the relevant enabled value.

Fair enough. However this change needs to be done in a different way. please check the header of the file you changed.

@Phenix66
Copy link
Author

My first thought is that there might be folks deploying this stack without alertmanager enabled but still wanting to get the dashboard for whatever reason. 🤔

I had a similar thought and debated implementing a new value similar to .Values.defaultRules but I decided against that since there were already other dashboards that simply relied upon the relevant enabled value.

Fair enough. However this change needs to be done in a different way. please check the header of the file you changed.

Updated to use the sync script

@GMartinez-Sisti
Copy link
Member

I'd like to get a second opinion on this, the change might be considered a breaking change because something that might be in use is removed use once this is merged. @jkroepke @QuentinBisson pls 🙏

@jkroepke
Copy link
Member

In general, I would favorite a more generic approach where individual dashboards can be toggled.

…e dashboard when AM is disabled

Signed-off-by: Phenix66 <34311559+Phenix66@users.noreply.github.com>
@Phenix66
Copy link
Author

In general, I would favorite a more generic approach where individual dashboards can be toggled.

Added an individual toggle identical to the one used for node exporter to do the same thing.

Signed-off-by: Phenix66 <34311559+Phenix66@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants