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.

Sign up for GitHub

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

Jump to bottom

Set propagate to False on "uvicorn" logger #1288

Merged
merged 6 commits into from Oct 8, 2022
Merged

Set propagate to False on "uvicorn" logger #1288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
32e24d0
Fix duplicate logs when use logging.basicConfig (#1285)
guyskk Dec 9, 2021
cb314ed
Workaround pytest caplog not able to capture no-propagate loggers
guyskk Dec 11, 2021
2d3213c
fix code style issues
guyskk Dec 11, 2021
121a176
update comment on tests logger propagate
guyskk Jul 6, 2022
c1cf628
Merge branch 'master' into fix-duplicate-logs
guyskk Jul 6, 2022
539ef90
fix lint comment whitespace
guyskk Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Expand Up @@ -15,6 +15,17 @@

from uvicorn.config import LOGGING_CONFIG

# Note: We explicitly turn the propagate on just for tests, because pytest
# caplog not able to capture no-propagate loggers.
#
# And the caplog_for_logger helper also not work on test config cases, because
# when create Config object, Config.configure_logging will remove caplog.handler.
#
# The simple solution is set propagate=True before execute tests.
#
# See also: https://github.com/pytest-dev/pytest/issues/3697
LOGGING_CONFIG["loggers"]["uvicorn"]["propagate"] = True
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then... all our users needs to do this if capturing the logs on their setup, right? 🤔

Should we mention this somewhere, or it's fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all our users needs to do this if capturing the logs on their setup, right?

I think if users want to config uvicorn's logs, they should provide full logging config, no needs to set propagate. https://www.uvicorn.org/settings/#logging

And if users only want to config application logs, call logging.basicConfig works as expected, uvicorn's logs will not affected.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, if people want to capture those logs on their test, for some reason (?) they need to do the same thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then... all our users needs to do this if capturing the logs on their setup, right? thinking

Should we mention this somewhere, or it's fine?

It introduces a change only for the users who in their tests, use a Config object and are looking for logs if I get it correctly, logging is complex so I'll admit I'm not 100% sure.

I'd be tempted to say it's a rather slim part of people in that case but I may be proven wrong, but there's no harm in saying something like:

Should you use the caplog pytest fixture in your test suite to capture uvicorn logs, you might want to use (the line above) if you use a Config object, please see (test line that explains it)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on this @guyskk ?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I just wanted to point this out, I don't have any strong opinion about this, jfyk)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, if people want to capture those logs on their test, for some reason (?) they need to do the same thing.

Yes. Maybe mention it in changelog? if those user's tests broken(rare usage), they can easily know why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok let's add a note then !



@pytest.fixture
def tls_certificate_authority() -> trustme.CA:
Expand Down
2 changes: 1 addition & 1 deletion uvicorn/config.py
View file
Open in desktop
Expand Up @@ -119,7 +119,7 @@
},
},
"loggers": {
"uvicorn": {"handlers": ["default"], "level": "INFO"},
"uvicorn": {"handlers": ["default"], "level": "INFO", "propagate": False},
"uvicorn.error": {"level": "INFO"},
"uvicorn.access": {"handlers": ["access"], "level": "INFO", "propagate": False},
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guyskk Is this "propagate": False still needed?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is.

},
Expand Down