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

fix: default logging #27777

Merged
merged 5 commits into from
Jun 25, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
@@ -47,6 +47,8 @@ assists people when migrating to a new version.
set `SLACK_API_TOKEN` to fetch and serve Slack avatar links
- [28134](https://github.com/apache/superset/pull/28134/) The default logging level was changed
from DEBUG to INFO - which is the normal/sane default logging level for most software.
- [27777](https://github.com/apache/superset/pull/27777) Moves debug logging logic to config.py.
See `LOG_LEVEL` in `superset/config.py` for the recommended default.
- [28205](https://github.com/apache/superset/pull/28205) The permission `all_database_access` now
more clearly provides access to all databases, as specified in its name. Before it only allowed
listing all databases in CRUD-view and dropdown and didn't provide access to data as it
4 changes: 2 additions & 2 deletions superset/config.py
Original file line number Diff line number Diff line change
@@ -893,15 +893,15 @@ class D3TimeFormat(TypedDict, total=False):
# Console Log Settings

LOG_FORMAT = "%(asctime)s:%(levelname)s:%(name)s:%(message)s"
LOG_LEVEL = logging.INFO
LOG_LEVEL = logging.DEBUG if DEBUG else logging.INFO

# ---------------------------------------------------
# Enable Time Rotate Log Handler
# ---------------------------------------------------
# LOG_LEVEL = DEBUG, INFO, WARNING, ERROR, CRITICAL

ENABLE_TIME_ROTATE = False
TIME_ROTATE_LOG_LEVEL = logging.INFO
TIME_ROTATE_LOG_LEVEL = logging.DEBUG if DEBUG else logging.INFO
FILENAME = os.path.join(DATA_DIR, "superset.log")
ROLLOVER = "midnight"
INTERVAL = 1
12 changes: 1 addition & 11 deletions superset/utils/logging_configurator.py
Original file line number Diff line number Diff line change
@@ -41,17 +41,7 @@ def configure_logging(
if app_config["SILENCE_FAB"]:
logging.getLogger("flask_appbuilder").setLevel(logging.ERROR)

# configure superset app logger
superset_logger = logging.getLogger("superset")
Copy link
Member

Choose a reason for hiding this comment

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

I have to admit I don't like this section that ties a bunch of behaviors to "debug_mode" potentially overriding LOG_LEVEL. It's like some configuration flags influence one another in a way that admins can't control.

I feel this needs more thinking/refactoring.

One option would be to move the DEBUG/LOG_LEVEL entanglement into superset/config.py so people can see that day LOG_LEVEL = logging.DEBUG if DEBUG else logging.INFO and choose to override it in their superset_config.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that's a very good idea. It is technically a breaking change, but only for debug-level environments so should be acceptable.

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 can move this addition to a separate PR if it requires further discussion.)

if debug_mode:
superset_logger.setLevel(logging.DEBUG)
else:
# In production mode, add log handler to sys.stderr.
superset_logger.addHandler(logging.StreamHandler())
Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear on what this does ^^^, but if it does anything that's not the default behavior, removing this line could affect people's production env and is a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the PR description under "Cause of duplicate logging" - by default logging.basicConfig triggers this, so this line causes logging duplication.

superset_logger.setLevel(logging.INFO)

logging.getLogger("pyhive.presto").setLevel(logging.INFO)
jessie-ross marked this conversation as resolved.
Show resolved Hide resolved

# basicConfig() will set up a default StreamHandler on stderr
logging.basicConfig(format=app_config["LOG_FORMAT"])
logging.getLogger().setLevel(app_config["LOG_LEVEL"])

Loading
Oops, something went wrong.