Add missing trace log for websocket protocols #1083
Conversation
There was a problem hiding this comment.
looks good, this would look better with some tests added, I think we may have some in test_logging.py !
now one thing I just realize reading what seem to be a copy of what we do in the http protocols is the following (bear with me it's been a while I did not touch this) : self.logger here is a uvicorn.error logger, however iirc when we set log_level to trace in Config, we end up with a middleware that spits out those trace log levels on the uvicorn.asgi logger. so should we put those under the uvicorn.asgi logger ? not sure if that makes sense.
no, correcting myself, those are good under that uvicorn.error logger, those are not strictly speaking asgi message where we put the one sent by the middlware
|
genuine question @laggardkernel : why did you need that ? |
What you mean by "that", this PR? |
yes I mean the ws trace messages :) |
|
@euri10 Not sure. Because there's only trace log for http protocol. It made me wonder if we forgot to add the same level log for ws. I did a searching about the history of trace log on http protocol. Logging on connection made, connection lost was first introduced in uvicorn 0.2.0, Later, with the coming of the Maybe @tomchristie could answer this, considering he's the guy designed the trace log and later integrated it into |
|
Ok I just wondered if there was a use case behind that and if you needed it to debug some sort of issue ! |
Formerly we only have trace log for http protocol. It seems we missed to add trace log for websocket protocols.
Protocols emit trace logs on `connection_made()` and `connection_lost`. Add missing tests for these logs.
The default transport protocol is http protocol. A ws request is firstly handled using http protocol, then handled using ws protocol by switching the protocol. To discriminate the trace log emitter, we need to specify the protocol type in the log message.
laggardkernel
force-pushed
the
feature/access-logging-for-ws
branch
from
9904f7d
to
d692c7e
Compare
There was a problem hiding this comment.
this looks beautiful, thanks a lot @laggardkernel and the benefit of having that trace level declared everywhere is that thre's no need to wait for the refactor PR merge !
* Add missing trace log for websocket protocols Formerly we only have trace log for http protocol. It seems we missed to add trace log for websocket protocols. * Add trace logging tests for protocols Protocols emit trace logs on `connection_made()` and `connection_lost`. Add missing tests for these logs. * Specify connection type in protocol trace log The default transport protocol is http protocol. A ws request is firstly handled using http protocol, then handled using ws protocol by switching the protocol. To discriminate the trace log emitter, we need to specify the protocol type in the log message.
* Add missing trace log for websocket protocols Formerly we only have trace log for http protocol. It seems we missed to add trace log for websocket protocols. * Add trace logging tests for protocols Protocols emit trace logs on `connection_made()` and `connection_lost`. Add missing tests for these logs. * Specify connection type in protocol trace log The default transport protocol is http protocol. A ws request is firstly handled using http protocol, then handled using ws protocol by switching the protocol. To discriminate the trace log emitter, we need to specify the protocol type in the log message.
Formerly we only have trace log for http protocols on
connection_madeandconnection_lostevents. Not very sure, but I guess we forgetto add trace log for websocket protocols?