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

[READY] adding alerts channels to the dashboards #37

Merged
merged 5 commits into from Oct 26, 2020
Merged

Conversation

ya7ya
Copy link
Contributor

@ya7ya ya7ya commented Oct 1, 2020

This PR is for coupling alerts with charts and have these alerts trigger in alertmanager (and subsequently pagerDuty)

why?

  • more visiblity on the dashboards, (the chart visually changes when an alert is triggering too)
  • quickly compare other stats using the explore function

@ya7ya ya7ya changed the title [WIP] adding alerts channels to the dashboards [READY] adding alerts channels to the dashboards Oct 6, 2020
@ya7ya ya7ya requested a review from iameli October 6, 2020 14:06
Copy link
Member

@iameli iameli left a comment

Choose a reason for hiding this comment

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

Awesome, I didn't really get how AlertManager and Grafana played together until now.

I want to make sure that the CPU and RAM alarms will trigger PagerDuty alerts. The previous things for that are here. Can we get the CPU/RAM alerts added there? Alternately, we could instead trigger PagerDuty alerts from the Grafana side if you think there's a way to do that.

@ya7ya
Copy link
Contributor Author

ya7ya commented Oct 13, 2020

@iameli that link you mentioned in the previous comment just leads me to the changed files, I'm not sure I follow .

If you mean why not just add it as a prometheus alert instead of grafana alert. if thats your point I can do that

@iameli
Copy link
Member

iameli commented Oct 14, 2020

Oh oops, I meant to link here. Those are the alerts that are currently capable of triggering PagerDuty.

If you mean why not just add it as a prometheus alert instead of grafana alert. if thats your point I can do that

I don't have any preference between Prometheus alerts and Grafana alerts really but any alerts should trigger our PagerDuty response and whatnot. So right now that means Prometheus, but I'd be also fine with figuring out how to integrate PagerDuty alerts from Grafana.

@ya7ya
Copy link
Contributor Author

ya7ya commented Oct 15, 2020

any alerts should trigger our PagerDuty response and whatnot. So right now that means Prometheus,

Yes , prometheus alertmanager is integrated with pagerDuty
the new grafana alerts is integrated by default with the alertmanager , so alerts would still trigger the pagerDuty because the alertmanager will forward these alerts.

The reason why use grafana alerts is because it makes it easier to look at the alert history on a chart and correlate that with other metrics, the alertmanager doesn't have that UX

I've added a direct pagerDuty integration too that we can activate that will allow grafana -> pagerDuty alerts without needing the alertmanager too. if thats more preferred I can activate it

@ya7ya ya7ya requested a review from iameli October 15, 2020 11:43
@ya7ya
Copy link
Contributor Author

ya7ya commented Oct 21, 2020

@iameli is this good to go? let me know if you have any changes to add here before we merge this.

@iameli
Copy link
Member

iameli commented Oct 23, 2020

Yep, LGTM!

@ya7ya ya7ya merged commit a823ea6 into master Oct 26, 2020
@hjpotter92 hjpotter92 deleted the ya/graf-prom-alerts branch May 31, 2023 05:50
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

2 participants