Conversation
catch exception raised and keep track of this metric
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| except ClientDisconnect: | ||
| log.info(f"Client disconnected mid-upload") | ||
| Metrics.CLIENT_DISCONNECT.inc() | ||
| return empty_measurement |
There was a problem hiding this comment.
We should verify how clients behave when we return empty_measurement instead of raising an exception, since this might affect changes in the client behavior.
I would actually suggest we preserve the original behavior by re-rasing the exception
There was a problem hiding this comment.
if we re-raise the exception it pollutes the logs with a Traceback; but if the client has disconnected - how would it receive anything?
There was a problem hiding this comment.
Yeah I think for the ClientDisconnect you are right. In the case of the catch-all exception, we ought to though return something that's an error to the client, either a 5xx error (by just re-raising the exception) or a specific 5xx/4xx status code.
| return empty_measurement | ||
| except Exception as e: | ||
| log.error(f"Uncaught exception {e}") | ||
| return empty_measurement |
There was a problem hiding this comment.
We should return something that indicates that this was not a successful submission; verify what repsonse should be sent. IE, should re-raise as a wrapped HTTPException?
on ClientDisconnect this won't be returned, but will suppress a noisy traceback in logs.
2 participants
catch exception raised and keep track of this metric #1116