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

monitoring: Prometheus default alerts #27596

Merged
merged 2 commits into from
Apr 29, 2019

Conversation

jan--f
Copy link
Contributor

@jan--f jan--f commented Apr 15, 2019

@sebastian-philipp sebastian-philipp changed the title Prometheus default alerts monitoring: Prometheus default alerts Apr 15, 2019
@jan--f jan--f force-pushed the prometheus-default-alerts branch 2 times, most recently from 8a06ee8 to f73fb02 Compare April 16, 2019 12:18
Signed-off-by: Jan Fajerski <jfajerski@suse.com>
Copy link

@Devp00l Devp00l left a comment

Choose a reason for hiding this comment

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

LGTM 👍

monitoring/prometheus/README.md Outdated Show resolved Hide resolved
monitoring/prometheus/README.md Outdated Show resolved Hide resolved
monitoring/prometheus/README.md Outdated Show resolved Hide resolved
@jan--f
Copy link
Contributor Author

jan--f commented Apr 23, 2019

@votdev I assumed you wanted full stops after all description texts, not just the 5 instances you pointed out?

@votdev
Copy link
Member

votdev commented Apr 23, 2019

@votdev I assumed you wanted full stops after all description texts, not just the 5 instances you pointed out?

Full stops are ok, too. I only wanted to see all descriptions ending the same way.

Copy link
Contributor

@LenzGr LenzGr left a comment

Choose a reason for hiding this comment

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

LGTM, please consider incorporating my textual enhancement suggestions.

@jan--f
Copy link
Contributor Author

jan--f commented Apr 24, 2019

As for packaging: I'm not sure it makes sense to add this to ceph.spec.in as a sub-package. Prometheus deployments are quite flexible, maybe it makes sense to leave this for downstream packagers?

@LenzGr
Copy link
Contributor

LenzGr commented Apr 24, 2019

As for packaging: I'm not sure it makes sense to add this to ceph.spec.in as a sub-package. Prometheus deployments are quite flexible, maybe it makes sense to leave this for downstream packagers?

Agreed - let's tackle this separately.

@LenzGr LenzGr requested a review from b-ranto April 24, 2019 12:05
@LenzGr
Copy link
Contributor

LenzGr commented Apr 24, 2019

@b-ranto - OK to be merged from your POV?

Copy link
Contributor

@b-ranto b-ranto left a comment

Choose a reason for hiding this comment

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

Sorry for the delay before finishing my review, I was pretty busy in the last couple of days.

Alerts are from
https://github.com/SUSE/DeepSea/blob/SES5/srv/salt/ceph/monitoring/prometheus/files/ses_default_alerts.yml
but updated for the mgr module and node_exporter >= 0.15.

Signed-off-by: Jan Fajerski <jfajerski@suse.com>
Copy link
Contributor

@b-ranto b-ranto left a comment

Choose a reason for hiding this comment

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

With the latest changes, lgtm.

@callithea
Copy link
Member

Jenkins test dashboard

@LenzGr LenzGr merged commit 106388b into ceph:master Apr 29, 2019
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