Skip to content
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

Closed
wants to merge 3 commits into from
Closed

Refactor uvicorn logger setup #1489

wants to merge 3 commits into from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented May 15, 2022

Changes

  • Add uvicorn.http and uvicorn.websockets loggers.
  • Replace uvicorn.error by uvicorn.server logger.

Missing

  • Docs update.
  • Check with UvicornWorker.

@Kludex Kludex added this to the Version 0.18.0 milestone May 15, 2022
@Kludex Kludex added the logging label May 15, 2022
@Kludex Kludex self-assigned this May 15, 2022
@Kludex Kludex force-pushed the refactor-logger-name branch 2 times, most recently from 8bb4b91 to b566f47 Compare May 15, 2022 17:52
@Kludex Kludex requested a review from a team May 15, 2022 17:55
Copy link
Contributor

@br3ndonland br3ndonland left a 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:

uvicorn/uvicorn/config.py

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
  • "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 to stderr?)

@Kludex
Copy link
Member Author

Kludex commented Jun 20, 2022

Did I get that right?

You did.

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.

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?

@br3ndonland
Copy link
Contributor

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.

@euri10
Copy link
Member

euri10 commented Jun 21, 2022

well I'd be in favor of NOT changing the name uvicorn.error

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 :)

@Kludex
Copy link
Member Author

Kludex commented Jun 21, 2022

I'm totally fine with not changing it. :)

@Kludex
Copy link
Member Author

Kludex commented Jun 21, 2022

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.

@Kludex Kludex closed this Jun 21, 2022
@Kludex Kludex deleted the refactor-logger-name branch June 21, 2022 18:10
@br3ndonland
Copy link
Contributor

That sounds good. I'm also fine with keeping the current logger name.

@doncatnip
Copy link

doncatnip commented Jun 23, 2022

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 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.

@alexsantos
Copy link

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
INFO: Uvicorn running on http://0.0.0.0:8000 (Press CTRL+C to quit).
For me, stderr has been always to log errors and not informational messages. For instance, starting uvicorn like this:
uvicorn main:app 2>error.log 1>out.log
will result in a file error.log with the start information.
I'm using Google Cloud Run and Cloud Functions and this might trigger alerts that aren't real errors.

@Kludex Kludex restored the refactor-logger-name branch October 27, 2022 09:15
@Kludex
Copy link
Member Author

Kludex commented Oct 27, 2022

I've changed my mind. I'm strong about this change now. I'm reopening this PR.

I'll update the documentation.

@Kludex Kludex reopened this Oct 27, 2022
@Kludex Kludex modified the milestones: Version 0.20.0, Version 0.21.0 Oct 29, 2022
@Kludex
Copy link
Member Author

Kludex commented Dec 25, 2022

I'm closing this as stale.

If someone wants to help me, and take this, I'd appreciate.

@Kludex Kludex closed this Dec 25, 2022
@Kludex Kludex deleted the refactor-logger-name branch December 25, 2022 17:40
@zanieb
Copy link

zanieb commented Dec 27, 2022

I'd consider taking a poke at this; what critical parts remain?

@Kludex
Copy link
Member Author

Kludex commented Dec 27, 2022

I think the "missing" section on the description. 👀

@ltbd78
Copy link

ltbd78 commented Apr 26, 2024

Would push for this change, as I dug into a rabbit hole of confusion seeing the logger named as uvicorn.error

Like a user said earlier, what if if we rename uvicorn.access touvicorn.client and uvicorn.error to uvicorn.client?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unfavorable logger name "uvicorn.error"
7 participants