Skip to content

TIMX 459 - update logging #362

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.

Sign up for GitHub

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

Jump to bottom
Merged
ghukill merged 2 commits into from
Feb 13, 2025
Merged

TIMX 459 - update logging #362

ghukill merged 2 commits into from
Feb 13, 2025

Conversation

@ghukill
Copy link
Contributor

@ghukill ghukill commented Feb 13, 2025 •
edited
Loading

Purpose and background context

This PR updates how logging is setup for the pipeline lambdas, with bulk of the changes in config.configure_logger(). Changes include:

  • make more explicit that configure_logger is modifying the root logger of the application
  • logging.basicConfig is no longer used, in favor of just setting a logging handler for the root logger
  • logging is no longer filtered to the application's loggers by default
    • this results in other loggers (e.g. 3rd party libraries, but also our own timdex_dataset_api (TDA) library) getting exposed
    • as these loggers can be chatty when in DEBUG mode, a list of loggers can optionally be passed that will receieve a much less verbose WARNING logging level

How can a reviewer manually see the effects of these changes?

First, set Dev1 TIMDEX credentials in console.

Next, perform a bulk update:

pipenv run tim --verbose bulk-update \
-s libguides \
--run-date="2025-02-12" \
--run-id="8489b541-f2bd-4d7f-bdf8-9d3fa82036cc" \
s3://timdex-extract-dev-222053980223/dataset/
  • note the very chatty logs, given the 3rd party DEBUG logs for botocore, opensearch, etc.
    • but recall, this is a feature not a bug! for the first time, these logs are available if helpful for debugging

Set the env var `` in .env file:

WARNING_ONLY_LOGGERS=botocore,opensearch

Re-run the bulk update:

pipenv run tim --verbose bulk-update \
-s libguides \
--run-date="2025-02-12" \
--run-id="8489b541-f2bd-4d7f-bdf8-9d3fa82036cc" \
s3://timdex-extract-dev-222053980223/dataset/
  • note the logs are quite manageable, though still DEBUG levels for TIM and TDA

Includes new or updated dependencies?

YES

Changes expectations for external applications?

NO

What are the relevant tickets?

Developer

  • All new ENV is documented in README
  • All new ENV has been added to staging and production environments
  • All related Jira tickets are linked in commit message(s)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

  • The commit message is clear and follows our guidelines (not just this PR message)
  • There are appropriate tests covering any new functionality
  • The provided documentation is sufficient for understanding any new functionality introduced
  • Any manual tests have been performed or provided examples have been verified
  • New dependencies are appropriate or there were no changes

@ghukill
Update logging, no filtering, explicit warning
7566cc2 
Why these changes are being introduced:

The previous approach filtered all logging to only loggers
from the namespace 'tim' thereby excluding the
new 'timdex_dataset_api' (TDA) logging that we would like
exposed.  This pattern works well most of the time when
3rd party library logging is not needed, but inflexible
when it is.

How this addresses that need:

Reworks the application logging setup as part of
configure_logger() to:

* not use logging.basicConfig()
* avoid filtering loggers, but instead explicitly setting
some logger names to WARNING level (e.g. chatty ones
like botocore) via arguments or env vars

Side effects of this change:

In production, where log level is INFO, it is possible
that some libraries may produce more logging statements,
but anticipated to be low. But desirable logs, like the
timdex_dataset_api library, will now show and will be
driven by this application's --verbose flag.

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-459
@ghukill
update dependencies
78d13f6
@ghukill ghukill requested a review from a team February 13, 2025 18:35
@ghukill ghukill marked this pull request as ready for review February 13, 2025 18:35
@ghukill ghukill merged commit 370f5b7 into main Feb 13, 2025
3 checks passed
@coveralls
Copy link

coveralls commented Feb 13, 2025

Pull Request Test Coverage Report for Build 13314429142

Details

  • 10 of 12 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.5%) to 99.545%
Changes Missing Coverage Covered Lines Changed/Added Lines %
tim/config.py 10 12 83.33%
Totals Coverage Status
Change from base Build 13207972226: -0.5%
Covered Lines: 438
Relevant Lines: 440
💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ehanson8 ehanson8 ehanson8 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ghukill @coveralls @ehanson8