kosteev commented on code in PR #30485:
URL: https://github.com/apache/airflow/pull/30485#discussion_r1158727447
##########
airflow/cli/commands/celery_command.py:
##########
@@ -96,6 +99,31 @@ def _serve_logs(skip_serve_logs: bool = False):
sub_proc.terminate()
+@after_setup_logger.connect()
+def logger_setup_handler(logger, **kwargs):
+ # Setup levels at which logs go to stderr and stdout if required
+ if conf.has_option("logging", "reset_and_split_logging") and conf.get(
+ "logging", "reset_and_split_logging"
+ ):
+ airflow_formatter = logging.Formatter(conf.get("logging",
"log_format"))
Review Comment:
Which formatter is actually used by Celery at the moment?
I think they have some different formatter by default, but if we are making
this change probably it is good idea to format Celery logs with same formatter.
then you can do settings.LOG_FORMAT here?
##########
airflow/cli/commands/celery_command.py:
##########
@@ -96,6 +99,31 @@ def _serve_logs(skip_serve_logs: bool = False):
sub_proc.terminate()
+@after_setup_logger.connect()
+def logger_setup_handler(logger, **kwargs):
+ # Setup levels at which logs go to stderr and stdout if required
+ if conf.has_option("logging", "reset_and_split_logging") and conf.get(
+ "logging", "reset_and_split_logging"
+ ):
Review Comment:
I think you can do it in one call like this which should be the same
```
if conf.get("logging", "reset_and_split_logging", fallback=None):
```
##########
airflow/config_templates/config.yml:
##########
@@ -870,6 +870,16 @@ logging:
type: string
example: "0o664"
default: "0o664"
+ reset_and_split_logging:
Review Comment:
This should somehow indicate that it is about Celery logs.
##########
airflow/cli/commands/celery_command.py:
##########
@@ -96,6 +99,31 @@ def _serve_logs(skip_serve_logs: bool = False):
sub_proc.terminate()
+@after_setup_logger.connect()
+def logger_setup_handler(logger, **kwargs):
+ # Setup levels at which logs go to stderr and stdout if required
+ if conf.has_option("logging", "reset_and_split_logging") and conf.get(
+ "logging", "reset_and_split_logging"
+ ):
+ airflow_formatter = logging.Formatter(conf.get("logging",
"log_format"))
+
+ class NoErrorOrAboveFilter(logging.Filter):
+ def filter(self, record):
+ return record.levelno <= logging.WARNING
+
+ below_error_handler = logging.StreamHandler(system_stdout)
+ below_error_handler.addFilter(NoErrorOrAboveFilter())
+ below_error_handler.setFormatter(airflow_formatter)
+
+ from_error_handler = logging.StreamHandler(system_stderr)
+ from_error_handler.setLevel(logging.ERROR)
+ from_error_handler.setFormatter(airflow_formatter)
+
+ logger.handlers.clear()
+ logger.addHandler(below_error_handler)
+ logger.addHandler(from_error_handler)
Review Comment:
Another way would be to do
```
logger.handlers[:] = [below_error_handler, from_error_handler]
```
(as done in _capture_task_logs method)
Since we access `logger.handlers` in `logger.handlers.clear()` anyway, then
above can be more concise version of achieving the same.
##########
airflow/config_templates/config.yml:
##########
@@ -870,6 +870,16 @@ logging:
type: string
example: "0o664"
default: "0o664"
+ reset_and_split_logging:
+ description: |
+ By default Celery sends all logs into stderr.
+ This option removes default handlers and provide new ones.
+ It might be useful to send low level logs like INFO and WARNING to
stdout,
Review Comment:
IMHO this doesn't sound very explicit that if it is enabled then logs with
WARNING level and below are sent to stdout .... "It might be useful" a bit
confusing for me
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
