Catch failures to contact remote homeserver for /register
#456
Conversation
|
Another sentry error comes from this request timing out. I'll extend this PR to handle that case too. |
DMRobertson
changed the title
/register
DMRobertson
force-pushed
the
dmr/sentry/register-report-bad-gateway
branch
from
40ebff3 to
c1c6067
Compare
| @@ -1,8 +1,9 @@ | |||
| from typing import BinaryIO, Optional, Type, TypeVar | |||
| from typing import BinaryIO, Optional, Sequence, Type, TypeVar | |||
There was a problem hiding this comment.
We should probably include license headers in the stubs files?
There was a problem hiding this comment.
Hmm---possibly? They feel more like metadata derived from twisted rather than original creation of ourselves. Not sure what the right thing to do from a licensing perspective is there.
| return { | ||
| "errcode": "M_UNKNOWN", | ||
| "error": "The Matrix homeserver did not include 'sub' in its response", | ||
| } |
There was a problem hiding this comment.
I think clients don't usually expect JSON bodies for 500 errors so something feels off to me?
I guess this is similar to what we had before but now we'll not error to sentry?
Do we want to log a warning or anything in this case?
There was a problem hiding this comment.
I think clients don't usually expect JSON bodies for 500 errors so something feels off to me?
Very possibly. I'm just trying to replicate what we had before (and also try to be a good citizen and provide error context).
I guess this is similar to what we had before but now we'll not error to sentry?
That's right. I didn't think it merited a sentry warning since it's not a problem in our application---it's duff data from the far end.
I think a warning would be prudent so that we'll have something in the logs to diagnose this. I added something for this on line 72. I'll add something for this and the cases below.
No more exceptions. Let's log something so it's not silent.
DMRobertson
force-pushed
the
dmr/sentry/register-report-bad-gateway
branch
from
ffbbc6c to
c23fc62
Compare
FWIW I'm not very happy with it either, but at least this is consistent with the existing behaviour. |
Like #456, but for another error case. If the remote homeserver fails to give us sensible response, that's not a problem in our application. We can continue to serve everyone else. We should tell the requester what happened and log a warning, but not an error to Sentry. Most of the examples I saw in Sentry were 404 responses serving up a generic "page not found" body.
* Catch failures to decode JSON in /register Like #456, but for another error case. If the remote homeserver fails to give us sensible response, that's not a problem in our application. We can continue to serve everyone else. We should tell the requester what happened and log a warning, but not an error to Sentry. Most of the examples I saw in Sentry were 404 responses serving up a generic "page not found" body.
Addresses https://sentry.matrix.org/sentry/sydent/issues/235004/?query=is%3Aunresolved%20event.timestamp%3A%3E%3D2021-11-03T10%3A00%3A00 That issue is rare---it's only occurred twice---but since it's so close to #456 I thought I may as well throw it in too. (I suppose this might encounter the problem in #463 as well, though we haven't seen it in Sentry. I think unbinds are just much less used that registers. I don't know if there's a better way to handle all this duplication though?)
Make it and the other "the remove homeserver did something silly" cases explicitly return 500. Log a warning message so that we can see the problem in logs, but not in sentry. This addresses the bulk of the errors spotted by matrix-org/matrix-spec#40.
The spec has nothing to say about status codes in the failure case. (matrix-org/matrix-doc#3470)