-
Notifications
You must be signed in to change notification settings - Fork 12k
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
Pause all alerts #6996
Pause all alerts #6996
Conversation
@@ -256,6 +256,7 @@ func Register(r *macaron.Macaron) { | |||
r.Group("/alerts", func() { | |||
r.Post("/test", bind(dtos.AlertTestCommand{}), wrap(AlertTest)) | |||
r.Post("/:alertId/pause", bind(dtos.PauseAlertCommand{}), wrap(PauseAlert), reqEditorRole) | |||
r.Post("/pause", bind(dtos.PauseAlertsCommand{}), wrap(PauseAlerts), reqGrafanaAdmin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not require just orgAdmin? the operation is only for org alerts, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ATM this is for all alerts in the instance.
I think both options make sense. But I think scheduling downtime/upgrades is something that grafana admins do rather then org admins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if's all alerts in the instance in needs to be a different HTTP api endpoint, /api/admin/ or something because /api/alerts only affects the current org .
And since we need a UI button for this in the alerts page I think making only act on the org alerts makes the most sense for now. I think the action for "ALL" alerts across multiple orgs does not make much sense, for that case I think we can have a "pause alert scheduler" or something :)
@@ -64,3 +64,8 @@ type PauseAlertCommand struct { | |||
AlertId int64 `json:"alertId"` | |||
Paused bool `json:"paused"` | |||
} | |||
|
|||
type PauseAlertsCommand struct { | |||
DataSourceIds []int64 `json:"datasourceId"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need data source ids here, this is starting to become very similar to the silence rules mechanic we have talked about (were we want silence rules with filters like data source, alert name regex, alert tags etc). The pause all alerts is more a temporary measure until we have a good silence mechanic and I was thinking of keeping it simple for that reason.
|
||
sess.Id(alert.Id).Update(&alert) | ||
if len(cmd.AlertIds) > 0 { | ||
buffer.WriteString(` WHERE id IN (?` + strings.Repeat(",?", len(cmd.AlertIds)-1) + `)`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure (but not 100%) that xorm supports this. have you tired WHERE id IN (?)
and then passing an array/slice as the param?
0e1c68b
to
d875c18
Compare
d875c18
to
a2257ec
Compare
@torkelo check again! :) |
AlertId int64 | ||
Paused bool | ||
OrgId int64 | ||
AlertIds []int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this be an array? seems to only pass single id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was an array when pausing per datasource was supported. When reverting that I was tempted to change it back to and int64. But keeping it as an array its easier to support multi pausing from the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see. And I guess that is potentially something we want, so we can keep that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill change to not using an array for clarity.
ref #6589