Alert ergonomics - #291
Conversation
There is no point in enforcing 'ack all'. Any ack shall ack.
If 'all' is passed instead of an incident number, snooze everything
0729558 to
830cd26
Compare
|
@rjbs Before I spend time testing the implementation, do you have any objections to the behavior change I'm proposing here? |
|
Nope! |
|
Testing seems ok: |
| my $res = await $self->_pd_request_for_user( | ||
| $event->from_user, | ||
| POST => "/incidents/$id/snooze", | ||
| { duration => $seconds } | ||
| ); | ||
|
|
||
| if (my $incident = $res->{incident}) { | ||
| my $title = $incident->{title}; | ||
| push @snoozed, "#$id ($title)"; | ||
| } else { | ||
| push @errors, $res->{message}; |
There was a problem hiding this comment.
_pd_request fails the future on non-2xx, so it never returns a hash with message — the else at 628 is unreachable, and the bare await propagates the failure out of the command. snooze all for 1h where #42 got resolved by someone else mid-batch: #41 stays snoozed in PD, #43 is never attempted, and the user just gets "My PagerDuty reactor crashed (in the background)".
| my $reply = sprintf("Snoozed incidents for %s: \n%s", duration($seconds), join("\n", @snoozed)); | ||
|
|
||
| return await $event->reply( | ||
| "Something went wrong talking to PagerDuty; they said: $msg" | ||
| ); | ||
| if (@errors) { | ||
| my $reply .= sprintf("\n\nUnfortunately we also received errors:\n%s", join("\n", @errors)); |
There was a problem hiding this comment.
my $reply at 635 declares a fresh variable inside the if, so the error text goes to a throwaway and never reaches the user.
| command ack => { | ||
| help => '*ack all*: acknowledge all triggered alerts in PagerDuty', | ||
| help => '*ack*: acknowledge all triggered alerts in PagerDuty', | ||
| } => async sub ($self, $event, $rest) { |
There was a problem hiding this comment.
$rest is bound here and never read again, so ack 42 acks the whole board silently — same for ack mine, ack banana. Bare ack meaning "all" is right, but acking all when trying to ack one is probably bad.
| # select a single incident if we | ||
| my @relevant = ($incident =~ /\d+/) ? grep {; $_->{incident_number} == $incident } @incidents : @incidents; |
There was a problem hiding this comment.
/\d+/ isn't anchored, so it's asking "is there a digit anywhere" rather than "is this an incident number". mine has no digit, falls through to the all-branch.
| snooze all for DURATION', | ||
| } => async sub ($self, $event, $rest) { | ||
| my ($num, $dur) = $rest =~ /^#?(\d+)\s+for\s+(.*)/i; | ||
| my ($incident, $dur) = $rest =~ /^#?(\S+)\s+for\s+(.*)/i; |
There was a problem hiding this comment.
A bare snooze gets $rest = undef from CommandPost, so this matches a regex against undef and probably logs a warning.
3 participants
Two changes to PagerDuty reactor commands: