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.

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

fix: default logging #27777

Merged
merged 5 commits into from
Jun 25, 2024
Merged

fix: default logging #27777

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ed26bf1
Simplify logging
jessie-ross Mar 29, 2024
527aa05
Remove unneeded logging.setLevel for pyhive.presto
jessie-ross Mar 29, 2024
1f808f3
Move debug logging logic to config.py
jessie-ross Jun 21, 2024
22ae88f
Add UPDATING instruction.
jessie-ross Jun 21, 2024
34007cc
Silence FAB logging
jessie-ross Jun 25, 2024
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions superset/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1180,7 +1180,7 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint: disable=invalid-name
# Whether to bump the logging level to ERROR on the flask_appbuilder package
# Set to False if/when debugging FAB related issues like
# permission management
SILENCE_FAB = True
SILENCE_FAB = not DEBUG
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we want this, do we? I think I put that in originally because it was littering the CLI with very verbose output...

I just tried setting it to False and got hundreds of lines like:

$ superset
Loaded your LOCAL configuration at [/Users/max/pythonpath/superset_config.py]
2024-06-21 13:25:21,890:WARNING:flask_appbuilder.security.manager:No user yet created, use flask fab command to do it.
2024-06-21 13:25:21,890:INFO:flask_appbuilder.base:Registering class SupersetIndexView on menu
2024-06-21 13:25:21,891:INFO:flask_appbuilder.baseviews:Registering route / ('GET',)
2024-06-21 13:25:21,891:INFO:flask_appbuilder.base:Registering class UtilView on menu
2024-06-21 13:25:21,891:INFO:flask_appbuilder.baseviews:Registering route /back ('GET',)
2024-06-21 13:25:21,891:INFO:flask_appbuilder.base:Registering class LocaleView on menu
2024-06-21 13:25:21,891:INFO:flask_appbuilder.baseviews:Registering route /lang/<string:locale> ('GET',)
2024-06-21 13:25:21,892:INFO:flask_appbuilder.base:Registering class SecurityApi on menu
2024-06-21 13:25:21,892:INFO:flask_appbuilder.api:Registering route /api/v1/security/login ['POST']
2024-06-21 13:25:21,892:INFO:flask_appbuilder.api:Registering route /api/v1/security/refresh ['POST']
2024-06-21 13:25:21,893:INFO:flask_appbuilder.base:Registering class ResetPasswordView on menu
2024-06-21 13:25:21,893:INFO:flask_appbuilder.baseviews:Registering route /resetpassword/form ['GET']
2024-06-21 13:25:21,893:INFO:flask_appbuilder.baseviews:Registering route /resetpassword/form ['POST']
2024-06-21 13:25:21,893:INFO:flask_appbuilder.base:Registering class ResetMyPasswordView on menu
2024-06-21 13:25:21,893:INFO:flask_appbuilder.baseviews:Registering route /resetmypassword/form ['GET']
2024-06-21 13:25:21,893:INFO:flask_appbuilder.baseviews:Registering route /resetmypassword/form ['POST']
2024-06-21 13:25:21,894:INFO:flask_appbuilder.base:Registering class UserInfoEditView on menu
2024-06-21 13:25:21,894:INFO:flask_appbuilder.baseviews:Registering route /userinfoeditview/form ['GET']
2024-06-21 13:25:21,894:INFO:flask_appbuilder.baseviews:Registering route /userinfoeditview/form ['POST']
2024-06-21 13:25:21,894:INFO:flask_appbuilder.base:Registering class AuthDBView on menu
2024-06-21 13:25:21,894:INFO:flask_appbuilder.baseviews:Registering route /login/ ['GET', 'POST']
2024-06-21 13:25:21,894:INFO:flask_appbuilder.baseviews:Registering route /logout/ ('GET',)
2024-06-21 13:25:21,896:INFO:flask_appbuilder.base:Registering class UserDBModelView on menu List Users
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/action/<string:name>/<pk> ['GET', 'POST']
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/action_post ['POST']
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/add ['GET', 'POST']
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/api/read ['GET']
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/delete/<pk> ['GET', 'POST']
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/edit/<pk> ['GET', 'POST']
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/list/ ('GET',)
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/show/<pk> ['GET']
2024-06-21 13:25:21,896:INFO:flask_appbuilder.baseviews:Registering route /users/userinfo/ ('GET',)
2024-06-21 13:25:21,899:INFO:flask_appbuilder.base:Registering class RoleModelView on menu List Roles
2024-06-21 13:25:21,899:INFO:flask_appbuilder.baseviews:Registering route /roles/action_post ['POST']
2024-06-21 13:25:21,899:INFO:flask_appbuilder.baseviews:Registering route /roles/add ['GET', 'POST']
2024-06-21 13:25:21,899:INFO:flask_appbuilder.baseviews:Registering route /roles/delete/<pk> ['GET', 'POST']
2024-06-21 13:25:21,899:INFO:flask_appbuilder.baseviews:Registering route /roles/edit/<pk> ['GET', 'POST']
2024-06-21 13:25:21,899:INFO:flask_appbuilder.baseviews:Registering route /roles/list/ ('GET',)
2024-06-21 13:25:21,899:INFO:flask_appbuilder.baseviews:Registering route /roles/show/<pk> ['GET']


FAB_ADD_SECURITY_VIEWS = True
FAB_ADD_SECURITY_PERMISSION_VIEW = False
Expand Down
12 changes: 1 addition & 11 deletions superset/utils/logging_configurator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"])

Expand Down
Loading