-
-
Notifications
You must be signed in to change notification settings - Fork 745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor uvicorn logger setup #1489
Conversation
8bb4b91
to
b566f47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Kludex, generally looks good.
Some users might be confused about the difference between "uvicorn.access"
vs. "uvicorn.http"
.
"uvicorn.access"
logs client-side HTTP access requests."uvicorn.http"
logs server-side HTTP protocol messages.
Did I get that right?
Do you think it might help to add some docs explaining what all the Uvicorn loggers do? Maybe on the server behavior page, cross-referencing it with the logging settings section? I'm happy to update the docs in a separate PR if that makes more sense.
Looking at the loggers we currently have:
Lines 411 to 415 in b566f47
logging.getLogger("uvicorn.server").setLevel(log_level) | |
logging.getLogger("uvicorn.access").setLevel(log_level) | |
logging.getLogger("uvicorn.http").setLevel(log_level) | |
logging.getLogger("uvicorn.websockets").setLevel(log_level) | |
logging.getLogger("uvicorn.asgi").setLevel(log_level) |
"uvicorn"
- Description: Parent/ancestor logger to which the child loggers (like
"uvicorn.server"
) will propagate. - File descriptor:
stderr
- Description: Parent/ancestor logger to which the child loggers (like
"uvicorn.server"
- Description: Logs messages related to core Uvicorn server functionality. (is there a better way to describe this?)
- File descriptor:
stderr
"uvicorn.websockets"
- Description: Logs server-side websocket messages.
- File descriptor:
stderr
"uvicorn.http"
- Description: Logs server-side HTTP protocol messages.
- File descriptor:
stderr
"uvicorn.access"
- Description: Logs client-side HTTP requests.
- File descriptor:
stdout
"uvicorn.asgi"
- Description: Used with
MessageLoggerMiddleware
to log low-level server-side ASGI messages, only for trace-level logging (Add 'trace' log level #474) - File descriptor:
stderr
(??? There's no handler configured, so I guess it defaults tostderr
?)
- Description: Used with
You did.
I'd be happy to receive help. You can use this branch if you want, so we can merge the functionality with the docs together (I'm not possessive with my branches 👀 ). But do we agree with those changes? I'm not sure about it myself... @br3ndonland @euri10 opinions? |
Ok cool! I will get some docs pushed up here soon. And definitely open to suggestions on the descriptions and such. I will defer to @euri10 for the direction we want to go overall. |
well I'd be in favor of NOT changing the name Not because I think the name is not confusing, it is. But because it mimics what gunicorn does with gunicorn.error` and dozens of users, not to say hundreds or even thousands, are on a daily basis using, seeing this same exact name, so really I don't get the fuss on this (last commit on https://github.com/benoitc/gunicorn/blob/master/examples/logging.conf is 20 feb 2014) : people literally use this name for almost ten years now.... I don't think we gain anything in changing something that people are most likely used to now for a long time. This said, I won't oppose it should people feel strongly about it, but to me there's almost no gain in changing, so I'll let you choose :) |
I'm totally fine with not changing it. :) |
Let's not do this then. I've closed #562. If someone has a good argument, and/or strategy to achieve it, feel free to comment on the issue. |
That sounds good. I'm also fine with keeping the current logger name. |
I don't feel very strongly about it, as I don't work with uvicorn anymore - but I do find the reasoning to keep the current scheme questionable at best. Gunicorn using a very confusing and in fact problem-inducing (i.e. log monitoring) naming scheme is not a good reason to propagate that anti-pattern further. I also wonder if the extend of the confusion can be compared. Uvicorn greets the user with "[gunicorn.error] Application startup complete" for example. |
For me as a newcomer it was really confusing to see a server starting and greeting me with a bunch of errors saying things like |
I've changed my mind. I'm strong about this change now. I'm reopening this PR. I'll update the documentation. |
b566f47
to
b347e69
Compare
bf3c940
to
e23c673
Compare
I'm closing this as stale. If someone wants to help me, and take this, I'd appreciate. |
I'd consider taking a poke at this; what critical parts remain? |
I think the "missing" section on the description. 👀 |
Would push for this change, as I dug into a rabbit hole of confusion seeing the logger named as Like a user said earlier, what if if we rename |
Changes
uvicorn.http
anduvicorn.websockets
loggers.uvicorn.error
byuvicorn.server
logger.Missing
UvicornWorker
.