-
Notifications
You must be signed in to change notification settings - Fork 30
bug: Log status_code & errno for all errors #462
Conversation
Commit message should be 'fix' instead of 'bug', and 'Closes #457', I don't believe fixes: like that will auto-close it. |
self._write_response(500, 999) | ||
|
||
def _overload_err(self, fail): | ||
"""errBack for throughput provisioned exceptions""" | ||
fail.trap(ProvisionedThroughputExceededException) | ||
self.log.info("Throughput Exceeded", **self._client_info) | ||
self.log.info("Throughput Exceeded", status_code=503, errno=201, | ||
**self._client_info) | ||
self._write_response(503, 201) | ||
|
||
def _router_response(self, response): |
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.
Is there any caller to _router_response that hasn't already logged out the 5xx?
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.
Well, _router_fail_err wasn't if exc.log_exception was set to False for whatever reason. You've got a point that if fail
isn't a RouterException, there's a real risk that we could be burying the real problem by trying to call log_exception
on a general exception. I'll try a few tests on that.
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.
Added an instance check in _router_fail_err. Obviously, if there's something else getting triggered, we'll need to address that, but hopefully we shouldn't trigger a 500 because of an invalid attribute reference.
Current coverage is 99.99%@@ master #462 diff @@
==========================================
Files 31 31
Lines 7454 7454
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 7453 7453
Misses 1 1
Partials 0 0
|
r+ |
fixes: #457
@bbangert r?