Skip to content

python: do not override root logger #7743

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

Merged
merged 3 commits into from
May 20, 2025
Merged

python: do not override root logger #7743

merged 3 commits into from
May 20, 2025

Conversation

isabelizimm
Copy link
Contributor

By adding a handler for "", we were effectively pushing all logs to a file, including the user's. I've removed the root logger and instead added a few other handlers to capture logs.

This change does expose us to a much higher likelihood of our logs leaking into the console.

Eg, the added handlers were after observing positron._vendor.pygls.protocol.json_rpc will send out logs if you run something like import xyzabc, and the Comm logs were leaking if there is an error in the comms.

Related issue in Jupyter, where they needed to handle tornado logs jupyter/notebook#1397

Release Notes

New Features

  • N/A

Bug Fixes

QA Notes

import logging
logging.warning("should show")

it's worth it to try to force a few other errors, to make sure we don't need to add other handlers.

Copy link

github-actions bot commented May 16, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical

readme  valid tags

@isabelizimm isabelizimm requested review from nstrayer and seeM and removed request for seeM and nstrayer May 19, 2025 16:12
@isabelizimm
Copy link
Contributor Author

going to make this a setting, as discussed 👾

seeM
seeM previously approved these changes May 20, 2025
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

@isabelizimm we could also try merging this change as is and try to catch any leaked loggers as they pop up especially given we're still in beta.

@isabelizimm isabelizimm merged commit 1936b9c into main May 20, 2025
28 checks passed
@isabelizimm isabelizimm deleted the ipython-console-logs branch May 20, 2025 16:16
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants