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

Update logging configuration #1775

Merged
merged 2 commits into from
Apr 11, 2024
Merged

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Apr 11, 2024

Description

Two changes to our logging configuration:

  • Update the LogLevel Enum to be a StrEnum, so it is easy to use the values from this enum to directly configure logging.
  • Update our logging handler creation to not set a log level.
    • We were setting the level on both the handlers, and on the logger itself. This is unnecessary, and makes it harder to update our log level when using celery.
    • This functionally doesn't change anything, since the log level is being controlled by the logger.

Motivation and Context

These changes were needed when configuring logging for Celery in #1760, but they stand on their own, and I think it makes sense for them to go in separately to simplify that PR.

How Has This Been Tested?

Running tests in CI.

Checklist

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@jonathangreen jonathangreen requested a review from a team April 11, 2024 15:19
Copy link

codecov bot commented Apr 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (75c30c0) to head (5d3b1bb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1775   +/-   ##
=======================================
  Coverage   90.23%   90.24%           
=======================================
  Files         247      247           
  Lines       28327    28346   +19     
  Branches     6462     6467    +5     
=======================================
+ Hits        25561    25580   +19     
  Misses       1829     1829           
  Partials      937      937           
Flag Coverage Δ
Api 75.77% <67.85%> (-0.01%) ⬇️
Core 59.47% <96.42%> (+0.02%) ⬆️
migration 24.12% <64.28%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

Looks good! 🎺

Comment on lines +42 to +45
debug = auto()
info = auto()
warning = auto()
error = auto()
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Can we move these up under the docstring, so that they are the most visible when looking at the class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately _generate_next_value_ has to be defined before any members.

https://docs.python.org/3/howto/enum.html#using-automatic-values

@jonathangreen jonathangreen merged commit 1895978 into main Apr 11, 2024
24 checks passed
@jonathangreen jonathangreen deleted the feature/logging-service-config branch April 11, 2024 16:09
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