Improve error reporting to users #3247
Conversation
readthedocs/projects/tasks.py
Outdated
| self.run_build(record=record, docker=docker) | ||
| failure = self.setup_env.failure or self.build_env.failure | ||
| except Exception as e: # noqa | ||
| log.exception('Top-level build exception has been raised') |
There was a problem hiding this comment.
This exception should not be logged?
There was a problem hiding this comment.
Why? Seems like we should definitely be logging it..
There was a problem hiding this comment.
@ericholscher Sorry, I could not explain it correctly.
I meant that this exception tracelog should not be reported to user.
There was a problem hiding this comment.
So we can add extra information while sending the data to sentry so that its easier to track.
log.exception('Top-level build exception has been raised', extra={"project": pk, "build": build_pk)There was a problem hiding this comment.
Yea, I think that's the plan now.
|
Also, we talked a bit more on this. I'm pretty generally -1 on reporting tracebacks to users, so this should explore another method of giving us a way to reference the exception to the user without giving a traceback. We talked about using Sentry to reference a message that we apply a message id to. We can continue on this next week. |
This should make us a lot more resilient to failures, specifically when *one* web server is acting up.
ericholscher
force-pushed
the
improve-task-error-reporting
branch
from
3da4cc0 to
243f82a
Compare
| @@ -421,7 +421,11 @@ def update_build(self, state=None): | |||
| self.build['error'] = str(self.failure) | |||
| else: | |||
| self.build['error'] = ugettext_noop( | |||
| "An unexpected error occurred") | |||
| "A failure in our code has occured. " | |||
There was a problem hiding this comment.
Nitpicks because apparently this is very commonly seen copy:
- "failure" -> "error": failure is a state we apply, error is what we hit
- "in our code" is very specific, where this can be any number of things we did. something more generic would be good here
Others:
"There was a problem with Read the Docs while building your documentation"
"We encountered an error with Read the Docs while building your documentation"
| "An unexpected error occurred") | ||
| "A failure in our code has occured. " | ||
| "Please report this to us with your build id ({}).".format( | ||
| self.build['pk'] |
There was a problem hiding this comment.
Not sure best way to handle this. Interpolation on localized strings should happen outside gettext. The interpolated variable should be named as well.
readthedocs/projects/tasks.py
Outdated
| # **Always** report build status. | ||
| # This can still fail if the API Is totally down, but should catch more failures | ||
| result = {} | ||
| error = 'Unknown error. Please include the build id ({}) in any bug reports.'.format( |
There was a problem hiding this comment.
- "Unknown error" -> "Unknown error encountered"
- Needs localization
- Named variable for interpolation
readthedocs/projects/tasks.py
Outdated
| 'success': False, | ||
| 'error': error, | ||
| } | ||
| result = api_v2.build(build_pk).patch(build_updates) |
There was a problem hiding this comment.
Reduce duplicated code to:
if hasattr(self, 'build'):
build_updates = {...}
else:
build_updates = {...}
return api_v2.build(build_pk).patch(build_updates)
Rebase of #3244