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.

Sign up for GitHub

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

Jump to bottom

Update test alert metadata #16026

Closed
wants to merge 1 commit into from
Closed

Update test alert metadata #16026

wants to merge 1 commit into from

Conversation

moolitayer
Copy link
Contributor

The purpose of this test alert is to see that alerts are configured correctly for going into ManageIQ.
This will make it easier to see alerts are working properly straight away

@openshift-merge-robot openshift-merge-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 29, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: moolitayer
We suggest the following additional approver: mfojtik

Assign the PR to them by writing /assign @mfojtik in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@mfojtik
Copy link
Contributor

mfojtik commented Aug 29, 2017

@smarterclayton ptal

@moolitayer
Copy link
Contributor Author

moolitayer commented Aug 29, 2017 •
edited

@zgalor @ilackarms Please review

@moolitayer
Copy link
Contributor Author

Pending some input from @zgalor I'm thinking this test alert has two options:

  • Have this example be unrelated to ManageIQ, so remove the existing ManageIQ bits (since it is misleading and will not work this way)
  • Have this example properly configured to be picked up by ManageIQ (current version)

cc @smarterclayton

@ilackarms
Copy link

looks good

@smarterclayton
Copy link
Contributor

I'm confused why are we relaxing a valid alert with a fake one? I expect these to be real alerts.

@openshift-merge-robot openshift-merge-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 30, 2017
@moolitayer
Copy link
Contributor Author

I'm confused why are we relaxing a valid alert with a fake one? I expect these to be real alerts.

Configuring ManageIQ to receive OpenShift alerts for Prometheus alerting is not trivial.
What I'm trying to do with the alert is let an operator know that everything is working.
I've updated the PR to reflect that

@openshift-merge-robot openshift-merge-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 30, 2017
@moolitayer moolitayer mentioned this pull request Aug 30, 2017
Merged
@moolitayer
Copy link
Contributor Author

moolitayer commented Aug 30, 2017 •
edited

@smarterclayton maybe we should add the new alert commented out?

@moolitayer
Copy link
Contributor Author

ping @smarterclayton

@smarterclayton
Copy link
Contributor

Why not set the annotations on the existing alert?

@moolitayer
Copy link
Contributor Author

@smarterclayton because I thought to have an example of a generic alert and a manageiq alert.
Would you prefer updating the one alert?

@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 3, 2017 via email

@moolitayer
Copy link
Contributor Author

But they are both real now. Would you prefer I update the existing one?

@gabemontero gabemontero mentioned this pull request Sep 21, 2017
Merged
openshift-merge-robot added a commit that referenced this pull request Sep 28, 2017
@openshift-merge-robot
Merge pull request #16495 from gabemontero/build-alert
f86e504 
Automatic merge from submit-queue.

prometheus alerts for openshift build subsystem

https://trello.com/c/RskNHpfh/1334-5-prometheus-alerts-for-build-metrics

A WIP initial pass at alerts for the openshift build subsystem

@openshift/devex @smarterclayton @zgalor @moolitayer @mfojtik ptal, defer if bandwidth dictates, and/or pull in others as you each deem fit

Disclaimers:
1) I'm still debating the pros/cons of these alerts with https://docs.google.com/document/d/199PqyG3UsyXlwieHaqbGiWVa8eMWi8zzAn0YfcApr8Q/edit#heading=h.2efurbugauf in mind

2) still debating the template parameters / defaults for the various thresholds ... I still have a to-do to revisit with ops contacts potential default values based on their existing zabbix monitoring

3) still debating the severity as well

4) based on the activity in #16026 I did not include the `miqTarget` annotation

I also removed the space in the existing alert name based on how I interpreted various naming conventions.

And other than the query on the alerts URI, the extended test changes stemmed from flakiness experienced during testing that was unrelated to the addition of the alerts.

thanks
@openshift-merge-robot
Copy link
Contributor

@moolitayer PR needs rebase

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2018
@moolitayer
Copy link
Contributor Author

Closing in favor of:
#17608

@moolitayer moolitayer closed this Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mfojtik mfojtik

@smarterclayton smarterclayton

Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@moolitayer @openshift-merge-robot @mfojtik @ilackarms @smarterclayton