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

Conversation

jessie-ross
Copy link
Contributor

@jessie-ross jessie-ross commented Mar 29, 2024

SUMMARY

This PR makes default logging work as expected:

  • Fixes duplicate logging.
  • Allows DEBUG level logging in production.
  • Makes logging respect LOG_LEVEL and LOG_FORMAT in config.
  • Sets the LOG_LEVEL to INFO to match what we currently hardcode (theoretically a breaking change.)

I'm not sure what the initial goals were but by default we are logging DEBUG level everything but superset, which is set to INFO level (without regard to LOG_LEVEL,) and logging superset a second time without regard to the LOG_FORMAT config setting. This doesn't seem desirable for anybody.

This change makes default logging work as expected.

Resolves #21646
I think also resolves #4785

Cause of duplicate logging:

Python loggers output via handlers, the issue is we are adding handlers twice, once here:

superset_logger.addHandler(logging.StreamHandler())

And unintuitively also when we call logging.basicConfig() which by default adds a StreamHandler on root:

logging.basicConfig(format=app_config["LOG_FORMAT"])

Minimum reproduction of the bug:

>>> import logging
>>> # No need to set BasicConfig, as logging.lastResort will be used by default
>>> logging.basicConfig()
>>> logging.warning('test')
WARNING:root:test
>>> sublogger = logging.getLogger('sublogger')
>>> sublogger.addHandler(logging.StreamHandler())
>>> sublogger.warning('test')
test
WARNING:sublogger:test
>>>

TESTING INSTRUCTIONS

Inspect startup log messages, trigger a log if need be.

ADDITIONAL INFORMATION

@jessie-ross jessie-ross changed the title fix: Duplicate logging fix: Fix default logging Mar 29, 2024
@jessie-ross jessie-ross changed the title fix: Fix default logging fix: default logging Mar 29, 2024
superset/utils/logging_configurator.py Show resolved Hide resolved
superset/config.py Outdated Show resolved Hide resolved
@jessie-ross
Copy link
Contributor Author

@dpgaspar Just a friendly reminder this PR is waiting for review

@jessie-ross
Copy link
Contributor Author

Looks like the default log level was changed in #28134, so there should hopefully be no more points of contention.

@mistercrunch
Copy link
Member

Looks like the changes from my PR aren't into this PR (more specifically this line for instance https://github.com/apache/superset/pull/28134/files#diff-c99ae4b2b09b756ab2189a99a9685229f9d12633fc2616c368ea869770f603bfR849), mind rebasing?

UPDATING.md Outdated Show resolved Hide resolved
superset/config.py Outdated Show resolved Hide resolved
@@ -41,19 +41,11 @@ 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.)

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.

@jessie-ross jessie-ross force-pushed the fix/duplicate-log-messages branch 2 times, most recently from 15ff439 to 527aa05 Compare June 21, 2024 05:03
@jessie-ross
Copy link
Contributor Author

@mistercrunch Thanks for the quick review! I've rebased and addressed your comments.

@rusackas
Copy link
Member

Re-running CI 🤞

@@ -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']

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

LGTM outside of the SILENCE_FAB change, let's keep it False!

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.75%. Comparing base (76d897e) to head (34007cc).
Report is 357 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #27777       +/-   ##
===========================================
+ Coverage   60.48%   83.75%   +23.26%     
===========================================
  Files        1931      518     -1413     
  Lines       76236    37617    -38619     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31505    -14609     
+ Misses      28017     6112    -21905     
+ Partials     2105        0     -2105     
Flag Coverage Δ
hive 48.90% <ø> (-0.27%) ⬇️
javascript ?
mysql 77.29% <ø> (?)
postgres 77.40% <ø> (?)
presto 53.50% <ø> (-0.30%) ⬇️
python 83.75% <ø> (+20.26%) ⬆️
sqlite 76.85% <ø> (?)
unit 59.20% <ø> (+1.57%) ⬆️

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
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the contribution!

@mistercrunch mistercrunch merged commit d74d3a8 into apache:master Jun 25, 2024
37 checks passed
eschutho pushed a commit that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants