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

Clean logging #83

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Clean logging #83

merged 2 commits into from
Sep 30, 2021

Conversation

tnixon
Copy link
Contributor

@tnixon tnixon commented Sep 28, 2021

Resolving issue #25

@tnixon tnixon linked an issue Sep 28, 2021 that may be closed by this pull request
@tnixon tnixon added the enhancement New feature or request label Sep 28, 2021
@lgtm-com
Copy link

lgtm-com bot commented Sep 28, 2021

This pull request introduces 1 alert and fixes 1 when merging 5291710 into bcdb401 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for 'import *' may pollute namespace

Copy link
Contributor

@rportilla-databricks rportilla-databricks left a comment

Choose a reason for hiding this comment

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

This looks great. Just a couple suggestions (shouldn't be a blocker):

  1. It would be great to validate log levels in a unit test (for example set logging.basicConfig(level=logging.WARNING)
    and validate that the info doesn't appear).

  2. Is it simple enough to add for Scala also?

@rportilla-databricks rportilla-databricks merged commit 1b43f53 into master Sep 30, 2021
@rportilla-databricks rportilla-databricks deleted the clean_logging branch September 30, 2021 02:46
@CTCC1
Copy link
Contributor

CTCC1 commented Oct 11, 2021

Hi there, just chiming-in as an end-user:
After this change, we can see the warnings log as follow in databricks:

WARNING:root:Column _c had no values within the lookback window. Consider using a larger window to avoid missing values. If this is the first record in the data frame, this warning can be ignored.

Note the "root" here.

https://docs.python.org/3.9/howto/logging.html#advanced-logging-tutorial
Based on my own experience, instead of log every line directly using logging.info, using a module level logger is preferred, such as:

logger = logging.getLogger(__name__)

Then the log lines will be more clear, and say sth like WARNING:tempo.tsdf, and user can also set logging level in their code, e.g.

import logging
logging.basicConfig(level=logging.INFO)
logging.getLogger("py4j").setLevel(logging.WARNING)
logging.getLogger("tempo").setLevel(logging.WARNING)

@tnixon
Copy link
Contributor Author

tnixon commented Oct 20, 2021

Good suggestions, @CTCC1 - I agree, we should update the logging accordingly!

@tnixon tnixon mentioned this pull request Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use logging library
3 participants