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

Improve logging #13735

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Improve logging #13735

wants to merge 25 commits into from

Conversation

zklaus
Copy link
Contributor

@zklaus zklaus commented Mar 26, 2024

Description

Fixes #13541

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 26, 2024
Copy link

codspeed-hq bot commented Mar 26, 2024

CodSpeed Performance Report

Merging #13735 will improve performances by 14.48%

Comparing zklaus:debug-logging (d389a49) with main (eb45954)

Summary

⚡ 2 improvements
✅ 19 untouched benchmarks

Benchmarks breakdown

Benchmark main zklaus:debug-logging Change
test_jlap_fetch 54.7 ms 47.8 ms +14.48%
test_jlap_fetch_file 56.8 ms 50.2 ms +13.26%

@dholth
Copy link
Contributor

dholth commented Apr 1, 2024

We should adjust the benchmark to ignore jlap test setup/teardown. We should cache or save a local dummy ssl certificate used for that test https server.

@zklaus zklaus marked this pull request as ready for review April 1, 2024 15:38
@zklaus zklaus requested a review from a team as a code owner April 1, 2024 15:38
@dholth dholth mentioned this pull request Apr 1, 2024
3 tasks
docs/source/dev-guide/deep-dives/logging.md Outdated Show resolved Hide resolved
docs/source/dev-guide/deep-dives/logging.md Outdated Show resolved Hide resolved
docs/source/dev-guide/deep-dives/logging.md Outdated Show resolved Hide resolved
zklaus and others added 2 commits April 24, 2024 15:20
@zklaus
Copy link
Contributor Author

zklaus commented May 2, 2024

I updated the documentation part of this PR to address @jezdez' comments. I think there is still value in the actual code changes; if you prefer not to add the deep dive, we can just remove that.

Comment on lines 144 to 146
# root gets level ERROR
# 'conda' gets level WARN and does not propagate to root.
initialize_root_logger()
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should remove initialize_root_logger, AFAIK this is the source of logging issues in downstream projects (e.g., conda/conda-build#5275 (comment)).

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 have simply removed initialize_root_logger. I think this is justified in light of the fact that the whole logging hierarchy starts with an additional "root", conda, which is configured separately.

@zklaus
Copy link
Contributor Author

zklaus commented May 15, 2024

pre-commit.ci autofix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

Debug code guarded with log.isEnabledFor(logging.DEBUG) is always run
5 participants