Repository navigation
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.
# Conflicts: # ooniapi/services/ooniprobe/src/ooniprobe/routers/reports.py
receive_measurement catches ClientDisconnect without importing it, so any exception while reading the body raised a NameError there instead of reaching either handler.
client_upload_disconnect now counts by the probe_cc in the report_id. Nothing has checked that cc when the body is read, so anything that isn't a known country code is counted as XX. Tests cover a disconnect, for each kind of cc, and any other error reading the body.
|
added a label with probe_cc so we can see which countries are disconnecting during upload |
This branch has not been deployed
2 participants
catch exception raised and keep track of this metric #1116