-
Notifications
You must be signed in to change notification settings - Fork 137
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
send failsafe message when payload is too big #116
Conversation
@coryvirok easy implementation |
Cool |
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') |
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.
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)
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.
Nice! I'll add that.
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) |
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.
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.
0b2e63f
to
cb5dcc1
Compare
}, | ||
'notifier': SETTINGS['notifier'], | ||
'custom': { | ||
'orig-uuid': uuid, |
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.
As a best practice, I would use underscores instead of hyphens for field names. They'll be easier to search using RQL.
7efc8f6
to
77134ae
Compare
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:
custom
attribute containing the UUID and host from the original payload