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

Fixing logger terminal error #211

Merged
merged 5 commits into from
Mar 21, 2024
Merged

Fixing logger terminal error #211

merged 5 commits into from
Mar 21, 2024

Conversation

matt-heery
Copy link
Contributor

This is a fix to the issue that occurs when running a script with this module imported:

Before:

If you imported this module, it would change the root logger, and how it formatted logs, including a context element in the message. Since the module wants to send a log file to a specific file it created two handlers, one for the console and one for the log file, with differing levels (amongst other changes). The context filter (which creates default context for any logger message that doesnt have one from root upwards) wasn't added two both handlers and just added to the logger (using logger.addFilter()). This lead to hundreds of logging errors where the console handler couldn't find context. This made the logs literally impossible to read in terminal and made development very difficult.

After:

I have looped through all handlers and have added the filter specifically to each handler. This was a fix I had to implement in the airflow-matrix-scraper repo by editing the root handler and doing the exact same thing, so hoping fix will carry over to here.

Copy link
Contributor

@Thomas-Hirsch Thomas-Hirsch left a comment

Choose a reason for hiding this comment

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

great stuff with the detailed request message!

I think this is ok to me, though there may be an issue with using the root logger specifically as it could interact with other packages. Could use some further testing, and may end up changing it so it doesn't ask for the root logger.

If you bump the version (in both pyproject.yaml and data_linter.init.py) and update the changelog, I'd be happy to approve as is

@matt-heery
Copy link
Contributor Author

yeah igy re the root logger - though i dont think this should change any of the other packages logs, just standardise them log shown in terminal to be the same as the one in the log file - havent tested loads so agree, and made the changes requested

@Thomas-Hirsch Thomas-Hirsch self-requested a review March 21, 2024 15:22
Copy link
Contributor

@Thomas-Hirsch Thomas-Hirsch left a comment

Choose a reason for hiding this comment

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

Approved, only like 2 months late...

@Thomas-Hirsch Thomas-Hirsch merged commit 53f6afb into main Mar 21, 2024
16 checks passed
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

2 participants