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.

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

send failsafe message when payload is too big #116

Merged

Conversation

ezarowny
Copy link
Contributor

@ezarowny ezarowny commented Jun 24, 2016

Clubhouse link: https://app.clubhouse.io/rollbar/story/9393/if-an-error-is-too-large-for-pyrollbar-to-report-nothing-shows-up-anywhere

Large Rollbar payloads can cause issues in pyrollbar. After this PR, two things will happen when encountering a large payload:

  1. An error is logged containing the original payload and the UUID from it
  2. A new payload is sent with the custom attribute containing the UUID and host from the original payload

@ezarowny
Copy link
Contributor Author

ezarowny commented Jun 24, 2016

@coryvirok easy implementation

@coryvirok
Copy link
Contributor

Cool

@ezarowny
Copy link
Contributor Author

cc @brianr

@@ -1217,6 +1229,7 @@ def _parse_response(path, access_token, params, resp, endpoint=None):
return
elif resp.status_code == 413:
log.warning("Rollbar: request entity too large. Payload was: %r", params)
_send_failsafe('Payload too large')
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a litttle more descriptive and useful? Customers are going to see this, not know what it is, and ask support. How about we change this text to:

Failsafe from pyrollbar: payload too large. Original payload may be found in your server logs by searching for the UUID.

and attach the following as metadata to the failsafe payload:

  • the UUID (first 255 characters, just to be safe)
  • the server.host value (first 255 characters, just to be safe)

Then, for the log message on line 1231, include the UUID at the beginning of the line:

log.warning("Rollbar: request entity too large for UUID %r\nPayload:\n%r", uuid, params)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I'll add that.

@ezarowny
Copy link
Contributor Author

ezarowny commented Jun 24, 2016

Updated the PR. How's this look?

try:
send_payload(payload, data.get('access_token'))
except FailsafeException as e:
log.error("Rollbar: request entity too large for UUID %r\n. Payload:\n%r", uuid, payload)
Copy link
Contributor

Choose a reason for hiding this comment

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

This log message should probably come from e.message or something since we don't necessarily know what caused the failsafe.

Another approach would be to create a PayloadTooLargeException which we could catch explicitly here.

@ezarowny ezarowny force-pushed the ezarowny/ch9393/if-an-error-is-too-large-for-pyrollbar-to branch 2 times, most recently from 0b2e63f to cb5dcc1 Compare June 28, 2016 17:57
},
'notifier': SETTINGS['notifier'],
'custom': {
'orig-uuid': uuid,
Copy link
Member

Choose a reason for hiding this comment

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

As a best practice, I would use underscores instead of hyphens for field names. They'll be easier to search using RQL.

@ezarowny ezarowny force-pushed the ezarowny/ch9393/if-an-error-is-too-large-for-pyrollbar-to branch from 7efc8f6 to 77134ae Compare June 28, 2016 18:50
@ezarowny ezarowny merged commit b277c19 into master Jun 28, 2016
@ezarowny ezarowny deleted the ezarowny/ch9393/if-an-error-is-too-large-for-pyrollbar-to branch June 28, 2016 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants