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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert logging file name modification #1543

Merged
merged 1 commit into from Jun 27, 2022
Merged

Revert logging file name modification #1543

merged 1 commit into from Jun 27, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Jun 25, 2022

Reported on b48d32d#commitcomment-76948401.

The problem is that users are relying on configuration files like that one. _logging.py shouldn't be private module if they are using it. Also, I do think it's meant for users to do that. 馃

Thoughts here @euri10 ?

Kludex referenced this issue Jun 25, 2022
* Modified logging module name to avoid clash with stdlib

* Renamed _logging to indicate privacy
@euri10
Copy link
Member

euri10 commented Jun 27, 2022

yep, I'd prefer we add a note to say that change will crash the custom logger, it crashed mine too, just need to change the name inside the custom yaml config from logging to _logging

@Kludex
Copy link
Sponsor Member Author

Kludex commented Jun 27, 2022

But then... We are making the user access a private module (even if not directly), and motivating them to do it? 馃

@euri10
Copy link
Member

euri10 commented Jun 27, 2022

But then... We are making the user access a private module (even if not directly), and motivating them to do it? thinking

fair enough, e could also put the formatters in a public uvicorn.formatters and let the _logging module private ?

@florimondmanca
Copy link
Member

florimondmanca commented Jun 27, 2022

Yes, let's keep a publicly-accessible place for the logging helpers we expose to users. I stumbled upon this myself trying to copy this over... #491 (comment) after upgrading to 0.18.1.

For minimum disruption, I would say we could:

  1. Add uvicorn.formatters.*
  2. Add uvicorn.logging.*, with a module-level DeprecationWarning with instruction to switch to uvicorn.formatters.*.

It's also possible #1426 wasn't a good idea in the first place, and this PR is legit in the current state. In what situations was the name uvicorn.logging clashing with the stdlib logging?

In all cases, I think we should add a test that imports the items meant for public logging API -- the ones that would end up in strings in logging configuration files.

@tjader
Copy link

tjader commented Jun 27, 2022

Naming the module uvicorn.formatters is more ambiguous thant uvicorn.logging in the sense that there are many things that can be formatted. If you pick up a Formatter from the uvicorn.logging module, you know what you're looking for.
I hope you don't go for implicitly hidden module name uvicorn._logging.
Also, the only way you will end up having a name clash with the standard module is if you have your current working directory inside the (installed) path of uvicorn, or if you have done something non-standard with your PYTHONPATH. Any other properly installation of uvicorn shouldn't have this issue.

Copy link
Member

@florimondmanca florimondmanca left a comment

Based on this discussion I'm also in favor of straight-up reverting #1426. 馃憤

Adding a small "try to import logging stuff" test case would make this PR even better, but IMO it's acceptable in its current state.

@euri10 euri10 merged commit eaf8b4d into master Jun 27, 2022
12 checks passed
@euri10 euri10 deleted the revert-logging-name branch Jun 27, 2022
@macieyng
Copy link

macieyng commented Jun 27, 2022

I'm no expert, but changing import logging to import logging as py_logging in uvicorn internally would be sufficient to avoid name clashing while preserving uvicorn.logging. WDYT?

@florimondmanca
Copy link
Member

florimondmanca commented Jun 27, 2022

IIUC doing import logging when working on Uvicorn, i.e. with the project root directory as the cwd, would import the std logging library.

You'd need to do from uvicorn import logging or from ..logging import ... to access Uvicorn's logging module, which is what we probably do?

That's why I was confused and asked in what cases the name clashing which motivated #1426 would occur.

@euri10
Copy link
Member

euri10 commented Jun 27, 2022

iirc some people were running python uvicorn/main.py but I'm not too sure now

@Kludex
Copy link
Sponsor Member Author

Kludex commented Jun 27, 2022

Based on this discussion I'm also in favor of straight-up reverting #1426. 馃憤

Adding a small "try to import logging stuff" test case would make this PR even better, but IMO it's acceptable in its current state.

I didn't get what test you want. Can you rephrase/exemplify?

@ant31
Copy link

ant31 commented Jun 27, 2022

@Kludex Ith ink it's about to add a quick test that uses "uvicorn.logging".
So in the case of future PR similar to 1426, modifying an important end-user interface this test would fail.

@florimondmanca
Copy link
Member

florimondmanca commented Jun 27, 2022

I didn't get what test you want. Can you rephrase/exemplify?

Something like this:

def test_logging_api() -> None:
    # These items may be referenced by users in logging configurations.
    from uvicorn.logging import DefaultFormatter  # noqa
    from uvicorn.logging import AccessFormatter  # noqa

@polyrand
Copy link

polyrand commented Jun 28, 2022

The logging -> _logging rename also broke some of my apps when updating to 0.18.0. I never had issues with uvicorn.logging. Using _logging may be confusing for some users, since it seems you're accessing a private module and not doing the right thing.

I think it may be worth mentioning submodule renames in the release notes, too.

polyrand added a commit to litements/litexplore that referenced this issue Jun 28, 2022
Uvicorn changed back to  `logging`:
encode/uvicorn#1543
@Kludex
Copy link
Sponsor Member Author

Kludex commented Jun 28, 2022

I'll make sure to mention those if it happens again. Thanks @polyrand :)

@polyrand
Copy link

polyrand commented Jun 28, 2022

I'll make sure to mention those if it happens again. Thanks @polyrand :)

Thank you all for the awesome work 鉂わ笍

ballenspectric added a commit to spectriclabs/elastic_datashader that referenced this issue Jun 29, 2022
CrsiX added a commit to hopfenspace/MateBot that referenced this issue Jun 29, 2022
br3ndonland added a commit to br3ndonland/inboard that referenced this issue Jul 31, 2022
This commit will add a pytest parameter that verifies log message output
after configuring logging with the Uvicorn log format. This parameter is
important for testing not only for the log message format, but also the
use of the `uvicorn.logging.DefaultFormatter` class and its module. The
module has been renamed from `uvicorn.logging` to `uvicorn._logging`,
then reverted back to `uvicorn.logging`. The test will help ensure that
the Uvicorn module path (`uvicorn.logging.DefaultFormatter`) is correct.

encode/uvicorn#1543
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants