notify: log PagerDuty v1 response on BadRequest #1481
Conversation
a693852 to
e6e1519
Compare
notify/impl.go
Outdated
| if statusCode/100 != 2 { | ||
| return (statusCode == 403 || statusCode/100 == 5), fmt.Errorf("unexpected status code %v", statusCode) | ||
| } else if statusCode == 400 && resp.Body != nil { |
There was a problem hiding this comment.
This won't work since a 400 statusCode would satisfy the first test at L613.
e6e1519 to
a5c01d7
Compare
There was a problem hiding this comment.
Thanks for looking into this @adamdecaf.
More general question: Why only include the body in the error message if the code is 400? Why not include it in general? Would that be to noisy?
//CC @simonpasquier
notify/impl.go
Outdated
| if statusCode == 400 && resp.Body != nil { | ||
| bs, err := ioutil.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return false, fmt.Errorf("unexpected status code %v | problem reading response: %v", statusCode, err) |
There was a problem hiding this comment.
We are using : instead of | to separate errors in the Alertmanager project.
notify/impl.go
Outdated
| if err != nil { | ||
| return false, fmt.Errorf("unexpected status code %v | problem reading response: %v", statusCode, err) | ||
| } | ||
| return false, fmt.Errorf("bad request: %v", string(bs)) |
There was a problem hiding this comment.
Maybe still worth printing the status code here?
|
@mxinden There only looks to be a body on 400. There's no 429 on the v1 endpoint and 403 is blank. https://v2.developer.pagerduty.com/v2/docs/trigger-events It might be noisy, but right now there's no real way to see what's wrong from the PD side. |
Signed-off-by: Adam Shannon <adamkshannon@gmail.com>
a5c01d7 to
d117c95
Compare
|
@adamdecaf sounds good to me. Printing the body is a good idea, and probably more signal than noise to most users. @simonpasquier @stuartnelson3 any further thoughts, otherwise I will go ahead and merge. |
|
@adamdecaf thanks for the contribution! |
Signed-off-by: Adam Shannon <adamkshannon@gmail.com>
Signed-off-by: Adam Shannon <adamkshannon@gmail.com>
Applies prometheus#1481 to the Pagerduty V2 retry path
We had a problem (our fault) and PagerDuty was sending back 400's, but it was hard to debug without the error response from PagerDuty.