Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Improve sentry reports w/out context #613

Closed
pjenvey opened this issue Aug 19, 2016 · 1 comment · Fixed by #621
Closed

Improve sentry reports w/out context #613

pjenvey opened this issue Aug 19, 2016 · 1 comment · Fixed by #621
Assignees
Labels

Comments

@pjenvey
Copy link
Member

pjenvey commented Aug 19, 2016

Some sentry reports lack context due to twisted Failures not including a traceback.

One cause is twisted.web.client's _WrapperException which wraps other exceptions (possibly even multiple exceptions) and commonly lack a tb. We should special case these: log their wrapped exceptions instead, which'll likely include a context.

e.g. ResponseNeverReceived is one:
https://sentry.prod.mozaws.net/operations/autopush-prod/issues/345377/

There's others without a tb for unknown reasons:
https://sentry.prod.mozaws.net/operations/autopush-prod/issues/345112/events/8063796/

We can special case the logger to forcefully include the current stack trace when coming across these. That might provide enough context to track down their cause so we can improve their handling.

@pjenvey pjenvey added the p1 label Aug 19, 2016
@pjenvey pjenvey added this to the PUSHSVC-0: quality milestone Aug 19, 2016
@pjenvey pjenvey self-assigned this Aug 19, 2016
@pjenvey
Copy link
Member Author

pjenvey commented Aug 19, 2016

Er, before special casing _WrapperExceptions inside the logger itself, let's ensure they're all trapped/handled for all our twisted.web.client calls. So they never propagate down to the logger in the first place.

(The logger special casing might still be useful in case we forget to handle new ones in the future).

pjenvey added a commit that referenced this issue Aug 22, 2016
it's sent as sentry's top level stacktrace vs its explicit exception
stacktrace

issue #613
pjenvey added a commit that referenced this issue Aug 22, 2016
it's sent as sentry's top level stacktrace vs its explicit exception
stacktrace

issue #613
jrconlin pushed a commit that referenced this issue Aug 22, 2016
it's sent as sentry's top level stacktrace vs its explicit exception
stacktrace

issue #613
pjenvey added a commit that referenced this issue Aug 23, 2016
* prefer base classes ConnectError/ConnectionClosed/ResponseFailed,
  covering ConnectionRefused/UserError, ConnectionLost/Done,
  ResponseNeverReceived
* handle ResponseFailed (ResponseNeverReceived) in the routers as
  a broken connection
* trap these in PushServerProtocol._notify_node

closes #613, #554
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants