HTTPStatus.* have started to appear in Processed request log lines instead of an integer

[anoadragon453]

An example log line from matrix.org:

2022-01-24 08:10:36,396 - synapse.access.http.8080 - 445 - INFO - GET-7050353 - xxx.xxx.xxx.xxx - 8080 - {@redacted:matrix.org} Processed request: 0.296sec/0.003sec (0.007sec, 0.000sec) (0.002sec/0.288sec/2) 36141B HTTPStatus.OK "GET /_synapse/admin/v1/event_reports HTTP/1.1" "aiohttp/0.2.1" [0 dbevts]

This seems to currently happening for EventReportsRestServlet, RoomStateRestServlet and RoomRestServlet. Perhaps other endpoints are affected, but none observed in the recent logs on matrix.org.

Notice the HTTPStatus.OK in the logs, whereas one would expect 200. This seems to happen as we simply cast the return code to a str:

synapse/synapse/http/site.py

Line 410 in 10a88ba

code = str(self.code)

However, str(HTTPStatus.OK) unhelpfully resolves to the str "HTTPStatus.OK"...

This could have implications for scripts that parse Synapse logs.

[DMRobertson]

Sorry, this has my fingerprints all over it.

[DMRobertson]

code = str(int(self.code)) would work when self.code: int or self.code: HTTPStatus. Or maybe it's just easier to ditch the status enum unilaterally?

[anoadragon453]

I do like using HTTPStatus, but perhaps we should int(HTTPStatus) before returning from servlets?

[babolivier]

If we're starting to lean towards using HTTPStatus, we probably should fix the logging code rather than each individual servlet?

[anoadragon453]

ServletCallback is currently defined as returning and int when returning a status code:

synapse/synapse/http/server.py

Lines 202 to 207 in cbd82d0

	# Type of a callback method for processing requests
	# it is actually called with a SynapseRequest and a kwargs dict for the params,
	# but I can't figure out how to represent that.
	ServletCallback = Callable[
	..., Union[None, Awaitable[None], Tuple[int, Any], Awaitable[Tuple[int, Any]]]
	]

We could extend this to include HTTPStatus, and then update all code that interacts with it to process both correctly.

@babolivier I agree that that would probably be less things to update (and to remember) than doing int(HTTPStatus.*) everywhere.

[DMRobertson]

ServletCallback is currently defined as returning and int when returning a status code:

FWIW note that IntEnum instances will inherit from int:

>>> import http
>>> isinstance(http.HTTPStatus.OK, int)
True

[richvdh]

we've had this before, btw: #7017