[GitHub] [airflow] c-wilson commented on pull request #7740: [AIRFLOW-7065] Add optional propagation of task std streams to console

[GitBox <gi...@apache.org>]

c-wilson commented on pull request #7740:
URL: https://github.com/apache/airflow/pull/7740#issuecomment-623164289


   Hi @ashb I made some trivial cleanup changes to the ESTaskHandler but in reality I cannot find a better way to implement its functionality with the current (slightly deep) logging hierarchical pattern:
   
   ```airflow.LogStreamHandler > logging.logger > airflow.TaskLogHandler > logger.LogHandler > stream```
   
   In this flow, the ESTaskHandler really is just injecting a JSON formatter before the standard out stream as far as I can see. Seems that the sin of the ESTaskHandler is overloading the stdout and file writing paths in the same class which is a bit surprising.
   
   It is maybe a thankless and non-urgent task, but it may be worth a reevaluation of how the logging environment is composed at a broader level. When you enter a task context, factory functions could compose real stdlib `logging.Handler` instances instead of the current subclassing pattern. These could be composed such that they are pointed at desired streams (file or console) and have the desired formatters and could be attached to the logger via logger.addHandler. I'm not sure that this _actually_ cleaner/better, the current implementation obviously works, and I'm not sure how important it is that it be maximally readable/extensible.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org