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

Better Jira error handling #140

Merged
merged 3 commits into from
Feb 17, 2023
Merged

Better Jira error handling #140

merged 3 commits into from
Feb 17, 2023

Conversation

alin-at-dfinity
Copy link
Contributor

  • Return HTTP 400 Bad Request for non-retriable errors. It is inaccurate, but will prevent alertmanager from retrying.
  • Turns out go-jira does actually produce useful error messages (and it consumes the response body in the process). Log that instead of the empty body.
  • Also include HTTP response status 429 among retriable errors.

This addresses a number of currently open issues, including #132, #97 and #53.

Signed-off-by: Alin Sinpalean [email protected]

free added 2 commits November 23, 2022 14:31
@free
Better Jira error handling
0a33c60 
* Return HTTP 400 Bad Request for non-retriable errors. It is inaccurate, but
  will prevent alertmanager from retrying.
* Turns out go-jira does actually produce useful error messages (and it consumes
  the response body in the process). Log that instead of the empty body.

Signed-off-by: Alin Sinpalean <[email protected]>
@free
Also include HTTP response status 429 among retriable errors.
fc7e5e1 
Signed-off-by: Alin Sinpalean <[email protected]>
@alin-at-dfinity alin-at-dfinity changed the title Free/better error handling Better Jira error handling Nov 23, 2022
@alin-at-dfinity
Copy link
Contributor Author

Tested locally with a user that does not have access to the project. Before:

level=error ts=2022-11-23T09:15:18.640419384Z caller=main.go:174 msg="error handling request" statusCode=500 statusText="Internal Server Error" err="JIRA request https://xxx.atlassian.net/rest/api/2/search?fields=summary%2Cstatus%2Cresolution%2Cresolutiondate&jql=project%3D%22ABC%22+and+labels%3D%22JIRALERT%7Bfa9f32ae1adb7ccb234b23be211ae1971ed0a151ea1851da949c2d17faa0e286149ff9cec475b2744894d52faa93e225cdc7efe580a8e538d5948e3a1e518b9c%7D%22+order+by+resolutiondate+desc&maxResults=2 returned status 400 Bad Request, body \"\"" receiver=jira-abc groupLabels="unsupported value type"

After:

level=error ts=2022-11-23T13:16:53.796275429Z caller=main.go:175 msg="error handling request" statusCode=400 statusText="Bad Request" err="JIRA request https://xxx.atlassian.net/rest/api/2/search?fields=summary%2Cstatus%2Cresolution%2Cresolutiondate&jql=project%3D%22ABC%22+and+labels%3D%22JIRALERT%7Bc2c09f9c14d39d81e67bf00ec0b041d3cfe3505c5ffe2f81b737272938a26246dac498b48c6c5dbc770f464f2b878b4678cbd716ace7d896ff7857e7d6c24dbd%7D%22+order+by+resolutiondate+desc&maxResults=2 returned status 400 Bad Request, error \"The value 'ABC' does not exist for the field 'project'.: request failed. Please analyze the request body for more details. Status code: 400\"" receiver=jira-abc groupLabels="unsupported value type"

It's not ideal, as go-jira includes both the (formatted) body ("The value 'ABC' does not exist for the field 'project'") and the text "Please analyze the request body for more details", but at least the underlying error is logged now. And Alertmanager is hopefully persuaded to not retry.

@free
Include both the go-jira error and the response body in errors. Somet…
6c129f7 
…imes go-jira consumes the body and includes it in its error, sometimes it doesn't.

Signed-off-by: Alin Sinpalean <[email protected]>
bwplotka
bwplotka approved these changes Feb 17, 2023
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks!

@bwplotka bwplotka merged commit 7e2356a into master Feb 17, 2023
@bwplotka bwplotka deleted the free/better-error-handling branch February 17, 2023 09:22
This was referenced Aug 2, 2023
Open
Merged
rufusnufus pushed a commit to KazanExpress/jiralert that referenced this pull request May 13, 2024
@alin-at-dfinity @free @rufusnufus
Better Jira error handling (prometheus-community#140)
17d1ac3 
* Better Jira error handling

* Return HTTP 400 Bad Request for non-retriable errors. It is inaccurate, but
  will prevent alertmanager from retrying.
* Turns out go-jira does actually produce useful error messages (and it consumes
  the response body in the process). Log that instead of the empty body.

Signed-off-by: Alin Sinpalean <[email protected]>

* Also include HTTP response status 429 among retriable errors.

Signed-off-by: Alin Sinpalean <[email protected]>

* Include both the go-jira error and the response body in errors. Sometimes go-jira consumes the body and includes it in its error, sometimes it doesn't.

Signed-off-by: Alin Sinpalean <[email protected]>

---------

Signed-off-by: Alin Sinpalean <[email protected]>
Co-authored-by: Alin Sinpalean <[email protected]>
rufusnufus added a commit to KazanExpress/jiralert that referenced this pull request May 13, 2024
@rufusnufus @alin-at-dfinity
@free @holger-waschke @jkroepke @dj-holgie @jtyr @longtomjr @jan-zajic @julianfbeck @twotired @nlgotz @bwplotka
feat: cherry-pick upstream commits (#3)
e01463c 
* Better Jira error handling (prometheus-community#140)

* Better Jira error handling

* Return HTTP 400 Bad Request for non-retriable errors. It is inaccurate, but
  will prevent alertmanager from retrying.
* Turns out go-jira does actually produce useful error messages (and it consumes
  the response body in the process). Log that instead of the empty body.

Signed-off-by: Alin Sinpalean <[email protected]>

* Also include HTTP response status 429 among retriable errors.

Signed-off-by: Alin Sinpalean <[email protected]>

* Include both the go-jira error and the response body in errors. Sometimes go-jira consumes the body and includes it in its error, sometimes it doesn't.

Signed-off-by: Alin Sinpalean <[email protected]>

---------

Signed-off-by: Alin Sinpalean <[email protected]>
Co-authored-by: Alin Sinpalean <[email protected]>

* disable update existing jira issues with parameter (prometheus-community#150)

* Bump all dependencies (prometheus-community#133)

Signed-off-by: Jan-Otto Kröpke <[email protected]>

Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Holger Waschke <[email protected]>

* parameter to disable update jira issues

Signed-off-by: Holger Waschke <[email protected]>
Signed-off-by: Holger Waschke <[email protected]>

* rename parameter to make it more clear and avoid double negation. fix bug with missing return value.

Signed-off-by: Holger Waschke <[email protected]>

* Update main.go

Signed-off-by: dvag-holger-waschke <[email protected]>

* Update notify.go

Signed-off-by: dvag-holger-waschke <[email protected]>

* Update main.go

Signed-off-by: dvag-holger-waschke <[email protected]>

* Update main.go

Signed-off-by: dvag-holger-waschke <[email protected]>

* Update main.go

Signed-off-by: dvag-holger-waschke <[email protected]>

* Update notify.go

Signed-off-by: dvag-holger-waschke <[email protected]>

* fix for notify test

Signed-off-by: Holger Waschke <[email protected]>

---------

Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Holger Waschke <[email protected]>
Signed-off-by: Holger Waschke <[email protected]>
Signed-off-by: dvag-holger-waschke <[email protected]>
Co-authored-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Holger Waschke <[email protected]>

* Adding getEnv templating function (prometheus-community#153)

Signed-off-by: Jiri Tyr <[email protected]>

* feat: add support for static jira labels (prometheus-community#154)

Signed-off-by: Herman Ewert <[email protected]>
Co-authored-by: Herman Ewert <[email protected]>

* Fix prometheus-community#146 (safe limit of 200 characters from group label value) (prometheus-community#147)

Signed-off-by: jzajic <[email protected]>

* doc(PAT): Adds doc for PAT usage (prometheus-community#155)

Signed-off-by: Julian Beck <[email protected]>

* truncate descriptions that exceed -max-description-length (default 32KB) (prometheus-community#165)

* truncate descriptions that exceed -max-description-length (default 32,768)

Signed-off-by: Jason Wells <[email protected]>

* Update main.go

size was off by 1

Signed-off-by: Jason Wells <[email protected]>

---------

Signed-off-by: Jason Wells <[email protected]>

* fix: 🐛 Fixes error message for doTransition to display the proper transition state (prometheus-community#176)

Signed-off-by: Nathan Gotz <[email protected]>

* search for existing issue in multiple projects (prometheus-community#162)

* search for existing issue in multiple projects

Signed-off-by: Jason Wells <[email protected]>

* Apply suggestions from code review

Co-authored-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Jason Wells <[email protected]>

---------

Signed-off-by: Jason Wells <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]>

* add Fingerprint field to Alert so that it may be used in templates (prometheus-community#152) (prometheus-community#163)

Signed-off-by: Jason Wells <[email protected]>

---------

Signed-off-by: Alin Sinpalean <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Holger Waschke <[email protected]>
Signed-off-by: Holger Waschke <[email protected]>
Signed-off-by: dvag-holger-waschke <[email protected]>
Signed-off-by: Jiri Tyr <[email protected]>
Signed-off-by: Herman Ewert <[email protected]>
Signed-off-by: jzajic <[email protected]>
Signed-off-by: Julian Beck <[email protected]>
Signed-off-by: Jason Wells <[email protected]>
Signed-off-by: Nathan Gotz <[email protected]>
Co-authored-by: Alin Sinpalean <[email protected]>
Co-authored-by: Alin Sinpalean <[email protected]>
Co-authored-by: dvag-holger-waschke <[email protected]>
Co-authored-by: Jan-Otto Kröpke <[email protected]>
Co-authored-by: Holger Waschke <[email protected]>
Co-authored-by: Jiri Tyr <[email protected]>
Co-authored-by: Herman <[email protected]>
Co-authored-by: Herman Ewert <[email protected]>
Co-authored-by: Jan Zajic <[email protected]>
Co-authored-by: Julian Beck <[email protected]>
Co-authored-by: Jason Wells <[email protected]>
Co-authored-by: Nathan Gotz <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alin-at-dfinity @bwplotka @free