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

🐛 Do not disable existing loggers #132

Merged
merged 2 commits into from
Dec 15, 2024
Merged

Conversation

kraftp
Copy link
Contributor

@kraftp kraftp commented Dec 5, 2024

The new uvicorn log configuration introduced in #95 and released in 0.0.6 disables existing loggers. Therefore, any custom loggers defined in a FastAPI application will no longer work after upgrading to 0.0.6. This is an undocumented breaking change.

This PR returns FastAPI to its previous behavior by not disabling existing loggers.

Here is a minimal reproduction of this issue:

from fastapi import FastAPI

import logging

app = FastAPI()

logging.basicConfig(
    level=logging.INFO,
    format='%(asctime)s - %(levelname)s - %(message)s'
)

logger = logging.getLogger(__name__)

@app.get("/")
def example_endpoint() -> str:
    logger.info("example")
    return "example"

Start this app with fastapi run main.py. Visit the endpoint. On 0.0.6, without this PR, the log line is not printed. On 0.0.5, or with this PR, it is.

@tiangolo
Copy link
Member

tiangolo commented Dec 6, 2024

Nice, thank you @kraftp! 🚀

Could you please add a test for this? The same example might work. I think these are the relevant pytest docs: https://docs.pytest.org/en/stable/how-to/logging.html

It should fail with the current code, and should be fixed with your PR. 🤓

Verified

This commit was signed with the committer’s verified signature.
patrick91 Patrick Arminio
@patrick91
Copy link
Contributor

I had some time so I've added a test 😊 it fails as expected when reverting the change:

CleanShot 2024-12-09 at 12 52 53@2x

@patrick91 patrick91 added the bug Something isn't working label Dec 9, 2024
Copy link
Contributor Author

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

Thank you! I'm sorry, I had an unexpectedly busy weekend and didn't have a chance to add the test.

@patrick91
Copy link
Contributor

@kraftp no worries at all 😊

@tiangolo tiangolo changed the title Do Not Disable Existing Loggers 🐛 Do not disable existing loggers Dec 15, 2024
Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Good catch, thanks @kraftp! 🚀

And thanks @patrick91 for the tests! 🙇

This will be available in the next release, in the next few hours, fastapi-cli 0.0.7 🎉

@tiangolo tiangolo merged commit ffe012e into fastapi:main Dec 15, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants