You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "jessie-ross (via GitHub)" <gi...@apache.org> on 2024/03/29 07:13:52 UTC
[PR] fix: Duplicate logging [superset]
jessie-ross opened a new pull request, #27777:
URL: https://github.com/apache/superset/pull/27777
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR fixes logging:
- Duplicate logging.
- Allows debug 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 of everything but Superset, which is set to `INFO` level, and logging Superset INFO and above twice without regard to the `LOG_FORMAT` config setting. This doesn't seem desirable for anybody.
This change makes default logging work as expected.
Resolves https://github.com/apache/superset/issues/21646
I think also resolves https://github.com/apache/superset/issues/4785
#### Cause of duplicate logging:
Python loggers output via handlers, the issue is we are adding handlers twice, once here:
https://github.com/apache/superset/blob/c0f8dfc7f99a0e83a8c9b3c083288c76f50aa662/superset/utils/logging_configurator.py#L50
And unintuitively also when we call logging.basicConfig() which by default adds a StreamHandler on root:
https://github.com/apache/superset/blob/c0f8dfc7f99a0e83a8c9b3c083288c76f50aa662/superset/utils/logging_configurator.py#L55
Minimum reproduction of the bug:
```python
>>> import logging
>>> logging.warning('test')
WARNING:root:test
>>> sublogger = logging.getLogger('sublogger')
>>> sublogger.addHandler(logging.StreamHandler())
>>> sublogger.warning('test')
test
WARNING:sublogger:test
>>>
```
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Inspect startup log messages, trigger a log if need be.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue:
- https://github.com/apache/superset/issues/21646
- https://github.com/apache/superset/issues/4785
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on PR #27777:
URL: https://github.com/apache/superset/pull/27777#issuecomment-2124784023
@dpgaspar Just a friendly reminder this PR is waiting for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1545358461
##########
superset/utils/logging_configurator.py:
##########
@@ -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")
- if debug_mode:
- superset_logger.setLevel(logging.DEBUG)
- else:
- # In production mode, add log handler to sys.stderr.
- superset_logger.addHandler(logging.StreamHandler())
- superset_logger.setLevel(logging.INFO)
-
- logging.getLogger("pyhive.presto").setLevel(logging.INFO)
Review Comment:
I was guessing this line was put in place due to too many `DEBUG` level logs coming from `pyhive.presto`, due to having a default root log level of `DEBUG` - which I'm guess is by accident after looking at this file.
I'm happy to put it back if that's not the case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Fix default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1544170091
##########
superset/utils/logging_configurator.py:
##########
@@ -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")
- if debug_mode:
- superset_logger.setLevel(logging.DEBUG)
- else:
- # In production mode, add log handler to sys.stderr.
- superset_logger.addHandler(logging.StreamHandler())
- superset_logger.setLevel(logging.INFO)
-
- logging.getLogger("pyhive.presto").setLevel(logging.INFO)
Review Comment:
This was only necessary because we had `DEBUG` as the default log level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1545358461
##########
superset/utils/logging_configurator.py:
##########
@@ -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")
- if debug_mode:
- superset_logger.setLevel(logging.DEBUG)
- else:
- # In production mode, add log handler to sys.stderr.
- superset_logger.addHandler(logging.StreamHandler())
- superset_logger.setLevel(logging.INFO)
-
- logging.getLogger("pyhive.presto").setLevel(logging.INFO)
Review Comment:
I was guessing this line was put in place due to too many `DEBUG` level logs coming from `pyhive.presto`, due to having a default root log level of `DEBUG` - which I'm guessing is by accident after looking at this file.
By setting the default `LOG_LEVEL` to `INFO` it becomes redundant.
I'm happy to put it back if that's not the case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1545358461
##########
superset/utils/logging_configurator.py:
##########
@@ -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")
- if debug_mode:
- superset_logger.setLevel(logging.DEBUG)
- else:
- # In production mode, add log handler to sys.stderr.
- superset_logger.addHandler(logging.StreamHandler())
- superset_logger.setLevel(logging.INFO)
-
- logging.getLogger("pyhive.presto").setLevel(logging.INFO)
Review Comment:
Well, the root log level was set to `DEBUG`, but the log level for `superset` was set to `INFO`.
I was guessing this line was put in place due to too many `DEBUG` level logs coming from `pyhive.presto`, due to having a default root log level of `DEBUG` - which I'm guess is by accident after looking at this file.
I'm happy to put it back if that's not the case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #27777:
URL: https://github.com/apache/superset/pull/27777#issuecomment-2182988120
Re-running CI 🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1544720655
##########
superset/config.py:
##########
@@ -841,7 +841,7 @@ class D3Format(TypedDict, total=False):
# Console Log Settings
LOG_FORMAT = "%(asctime)s:%(levelname)s:%(name)s:%(message)s"
-LOG_LEVEL = "DEBUG"
+LOG_LEVEL = "INFO"
Review Comment:
Would you mind adding a line item to `UPDATING.md`?
##########
superset/utils/logging_configurator.py:
##########
@@ -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")
- if debug_mode:
- superset_logger.setLevel(logging.DEBUG)
- else:
- # In production mode, add log handler to sys.stderr.
- superset_logger.addHandler(logging.StreamHandler())
- superset_logger.setLevel(logging.INFO)
-
- logging.getLogger("pyhive.presto").setLevel(logging.INFO)
Review Comment:
@jessie-ross why is that? Any institution can override the default config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1545360981
##########
superset/config.py:
##########
@@ -841,7 +841,7 @@ class D3Format(TypedDict, total=False):
# Console Log Settings
LOG_FORMAT = "%(asctime)s:%(levelname)s:%(name)s:%(message)s"
-LOG_LEVEL = "DEBUG"
+LOG_LEVEL = "INFO"
Review Comment:
Done in https://github.com/apache/superset/pull/27777/commits/85ca2cd9f8d5148eb90232e4672b939b515c45fb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1648418971
##########
superset/utils/logging_configurator.py:
##########
@@ -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")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on PR #27777:
URL: https://github.com/apache/superset/pull/27777#issuecomment-2182017060
@mistercrunch Thanks for the quick review! I've rebased and addressed your comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: default logging [superset]
Posted by "jessie-ross (via GitHub)" <gi...@apache.org>.
jessie-ross commented on code in PR #27777:
URL: https://github.com/apache/superset/pull/27777#discussion_r1545358461
##########
superset/utils/logging_configurator.py:
##########
@@ -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")
- if debug_mode:
- superset_logger.setLevel(logging.DEBUG)
- else:
- # In production mode, add log handler to sys.stderr.
- superset_logger.addHandler(logging.StreamHandler())
- superset_logger.setLevel(logging.INFO)
-
- logging.getLogger("pyhive.presto").setLevel(logging.INFO)
Review Comment:
I was guessing this line was put in place due to too many `DEBUG` level logs coming from `pyhive.presto`, due to having a default root log level of `DEBUG` - which I'm guessing is by accident after looking at this file.
I'm happy to put it back if that's not the case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org