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

Avoid uvicorn logger and use __main__ logger and remove logger from multiprocessing code #320

Merged
merged 8 commits into from
Jun 19, 2024

Conversation

northwestwitch
Copy link
Member

Description

Added/Changed/Fixed

How to test

  • Deploy and check how it goes!

Expected outcome

  • Hopefully it will work

Review

  • Tests executed by
  • "Merge and deploy" approved by

This version is a

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

import subprocess
from multiprocessing import Manager, Pool
from typing import Dict, List, Tuple

LOG = logging.getLogger("uvicorn.access")
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't used anyway

@northwestwitch northwestwitch changed the title Avoid uvicorn logger and use __main__ logger instead Avoid uvicorn logger and use __main__ logger and remove logger from multiprocessing code Jun 19, 2024
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -28,6 +30,7 @@ def create_db_and_tables():

@asynccontextmanager
async def lifespan(app_: FastAPI):
configure_log()
Copy link
Member Author

Choose a reason for hiding this comment

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

Call a function that configures the log before doing the logging

@northwestwitch northwestwitch requested a review from Jakob37 June 19, 2024 06:28
@Jakob37
Copy link
Collaborator

Jakob37 commented Jun 19, 2024

Nice! I'll see if I can reproduce the error locally. Otherwise I could perhaps put it on our server and we'll know next week (or after the vacation :) ).

@northwestwitch
Copy link
Member Author

Otherwise I could perhaps put it on our server and we'll know next week (or after the vacation :) ).

Exactly! ;)

@Jakob37
Copy link
Collaborator

Jakob37 commented Jun 19, 2024

I didn't manage to reproduce the issue on my laptop, so testing on the server it is!

Would it be fine to merge the latest master into this PR? Then I could production-test the d4tools update as well.

@northwestwitch
Copy link
Member Author

Would it be fine to merge the latest master into this PR? Then I could production-test the d4tools update as well.

The d4tools fix is merged in this branch. So it should be fine to test

@northwestwitch
Copy link
Member Author

I'm merging this PR seems it seems to work (tested on our dev server). Let's see how it goes after next weekend 🤞🏻

@northwestwitch northwestwitch merged commit 9ce94bd into main Jun 19, 2024
7 checks passed
@northwestwitch northwestwitch deleted the simpler_logger branch August 5, 2024 10:15
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.

2 participants