python-websockets / websockets

Library for building WebSocket servers and clients in Python
https://websockets.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
5.16k stars 513 forks source link

Logging Guide: LoggerAdapter logic error #1398

Closed louking closed 1 year ago

louking commented 1 year ago

I didn't see this issue raised after searching for "log" and for "doc" so hopefully this isn't a duplicate.

In https://websockets.readthedocs.io/en/stable/topics/logging.html under Configure Logging, there's the class

class LoggerAdapter(logging.LoggerAdapter):
    """Add connection ID and client IP address to websockets logs."""
    def process(self, msg, kwargs):
        try:
            websocket = kwargs["extra"]["websocket"]
        except KeyError:
            return msg, kwargs
        xff = websocket.request_headers.get("X-Forwarded-For")
        return f"{websocket.id} {xff} {msg}", kwargs

When using this to work with the client, the line xff = websocket.request_headers.get("X-Forwarded-For") will raise an exception during the connect process as request_headers isn't yet an attribute of websocket.

I realize the example is for the server, but this class can easily be made to work for the client by replacing that line with something like

        if getattr(websocket, 'request_headers', None):
            xff = websocket.request_headers.get("X-Forwarded-For")
        else:
            xff = '??'
aaugustin commented 1 year ago

Typical usage is writing a server or writing a client — but not both in the same code base.

That LoggingAdapter doesn't make sense for a client. I don't want to modify it to support copy pasting it in an inappropriate content without understanding what it does without crashing.

I would consider providing a sensible example for a client. What's your use case for a logging adapter for a client?

louking commented 1 year ago

I have two process talking with each other via websockets. One (the client) is collecting data from a serial device and forwarding to the webserver (the server). Occasionally a message to the server is lost, and I'm logging to see if I can find the reason.

aaugustin commented 1 year ago

Since these are two separate processes, each of them can get their own logging setup. You don't have to reuse the same code. So I'm still struggling to understand why you have that LoggerAdapter in your client code.

louking commented 1 year ago

Right. The LoggerAdapter probably isn't necessary as there's no way to have multiple websockets from the client. I'll remove it.

Thanks for your attention. I'll close this now.