[AIRFLOW-1611] Customize logging config #2631

Fokko · 2017-09-25T20:52:39Z

Dear Airflow maintainers,

The current way of loading a custom logging config using the logging_config_path does not work:
https://codecov.io/gh/Fokko/incubator-airflow/src/master/airflow/settings.py#L180

Therefore I would like suggest to change this so we can import a logger configuration that is defined in Python using a dict. The logging_config_path property in the config file can be used to point to a custom logging configuration which will be imported.

Change the configuration of the logging to make use of the python logging and make the configuration easy configurable. Some of the settings which are now not needed anymore since they can easily be implemented in the Python config dict itself.

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Airflow JIRA issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-1611

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

codecov-io · 2017-09-25T21:18:33Z

Codecov Report

Merging #2631 into master will decrease coverage by 0.26%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master    #2631      +/-   ##
==========================================
- Coverage   71.75%   71.49%   -0.27%     
==========================================
  Files         152      153       +1     
  Lines       11749    11745       -4     
==========================================
- Hits         8431     8397      -34     
- Misses       3318     3348      +30

Impacted Files	Coverage Δ
airflow/config_templates/airflow_local_settings.py	`100% <ø> (ø)`
airflow/logging_config.py	`100% <100%> (ø)`
airflow/settings.py	`92.53% <100%> (+3.4%)`	⬆️
airflow/www/app.py	`96.47% <100%> (ø)`	⬆️
airflow/jobs.py	`77.7% <50%> (+0.09%)`	⬆️
airflow/utils/log/s3_task_handler.py	`0% <0%> (-26.87%)`	⬇️
airflow/utils/log/gcs_task_handler.py	`0% <0%> (-23.81%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f0798f...e99f80c. Read the comment docs.

ashb · 2017-09-26T13:06:44Z

UPDATING.md

+
+Airflow's logging has been rewritten to uses Python’s builtin `logging` module to perform logging of the application. By extending classes with the existing `LoggingMixin`, all the logging will go through a central logger. The main benefit is easier configuration of the logging by setting a single python file. Disclaimer; there is still some inline configuration, but this will be removed eventually.
+
+The loggers and handlers can be adjusted by placing a python file on the filsystem, and point the `logging_config_path` setting in your `airflow.cfg` to the configuration file. Please note that the variable that contains the config needs to be names `LOGGING_CONFIG`:


be named (vs names)

ashb · 2017-09-26T13:11:01Z

airflow/config_templates/default_airflow_local_settings.py

-# directly specify in the handler definitions. This is to
-# provide compatibility to older remote log folder settings.
-REMOTE_BASE_LOG_FOLDER = conf.get('core', 'REMOTE_BASE_LOG_FOLDER')
-S3_LOG_FOLDER = ''


I think for 1.9 we should have this (and others in this diff) still be present but deprecated.

I understand that this is a big change, but by keeping this, we're not really cleaning up the legacy.

ashb · 2017-09-26T13:13:27Z

airflow/logging_config.py

+        path = os.path.dirname(config_path)
+        sys.path.append(path)
+
+        filename = os.path.basename(config_path).replace('.py', '')


Does this actually work if it contains '/' in it? Or should this setting be called loging_config_module?

i.e. conf/mylogging.py vs conf.mylogging?

The custom config filename can be anything, but it has to be a python file because we expect a python dict, so conf/mylogging.py would be fine.

Oh right, I see how it's done, because we add the directory of the file to python path.

That might lead to some counter-intuitive behaviour if my config module is "deep" in another module somewhere (say mycompany.airflow.custom_logging) which would then add mycompany/airflow/ into the python search path, meaning if it did it's own imports of mycompany.x, they wouldn't work.

At the very least we need to document what we do with the python import path, though my preference here would be to leave any altering of the python path up to the user (via PYTHONPATH env var) and to accept something like what gunicorn does: $(MODULE_NAME):$(VARIABLE_NAME) (where VARIABLE_NAME could instead be hard-coded to be LOGGING_CONFIG)

Keep in mind that it is appended to the path, not prepended. So it will not alter any existing imports. The only problem might be that the import is already existing.

What would you recommend? Before, I searched for a specific file on the PYTHONPATH. But this might be a bit more cumbersome for some users. On @bolkedebruin's suggestion I changed it so you now can set the path to the config file: logging_config_path=/home/fokko/airflow_config.py.

Oh yes, good point about adding to the end of the path, not the start.

But here's a thought that would make this a smaller change, and mean we don't have to load another file. Given that airflow already loads 'airflow_local_settings.py', we could define a LOGGING_CONFIG dict in airflow.settings, and then use that, rather than a new python file just for logging.

i.e:

airflow.settings module defines the base/default LOGGING_CONFIG in dict form

due to the already existing from airflow_local_settings import * if that defines a new LOGGING_CONFIG it would be used instead.

configure_logging() could then just look at airflow.settings.LOGGING_CONFIG.

Thoughts?

(Forgive me if I missed some discussions about this, I was on vacation)

Thank you for the feedback. I looked into how Django is doing this, and learned from here that they just import a module. So in the airflow.cfg you'll have to specify:

LOGGING_CONFIG_CLASS = my.path.class.LOGGING_CONFIG

Which has to be on the PYTHONPATH, but Airflow will just search for this class and import the LOGGING_CONFIG dict, which is then loaded. What do you think about this?

Yeah that sounds like a sensible approach to take, (and copying from Django is almost always a good idea to me 😄)

ashb · 2017-09-26T13:14:24Z

airflow/logging_config.py

+        )
+    except AttributeError:
+        # Import default logging configurations.
+        log.info(


This is probably a warning rather than info, as it is likely a mistake

Good suggestion, thanks!

ashb · 2017-09-26T13:15:52Z

airflow/logging_config.py

+        config_path = conf.get('core', 'logging_config_path')
+
+    try:
+        path = os.path.dirname(config_path)


The default for logging_config_path is an empty string, which I think causes the "Unable to load custom logging, using default config instead" message to be printed (I think)

It would be better to print nothing in the case of the default setting and just use the stock config without logging about it.

What about setting the logging level to debug for this?

ashb · 2017-09-26T13:19:39Z

I'm not sure that we have the correct upgrade path from 1.8.x here -- i.e. if I am currently using S3 remote logging and I upgrade to this 1.9 it will silently stop working.

I do like the idea of the change though.

bolkedebruin · 2017-09-27T21:02:57Z

airflow/logging_config.py

+            # Make sure that the variable is in scope
+            assert (isinstance(logging_config, dict))
+
+            log.info(


Use .format

We use this way of formatting the logging messages throughout Airflow:
#2592

bolkedebruin · 2017-09-27T21:03:18Z

airflow/logging_config.py

-        )
-        from airflow.config_templates.default_airflow_local_settings import \
+    if logging_class_path == '':
+        from airflow.config_templates.airflow_local_settings import \


Please use () for multiline

AFAIK this is not working for imports:

[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> (from math import exp) File "<stdin>", line 1 (from math import exp) ^

The things to import go in the brackets:

from math import ( exp as POW, )

Thanks, love it

bolkedebruin · 2017-09-27T21:04:45Z

airflow/logging_config.py

-        # Make sure that the variable is in scope
-        assert (isinstance(logging_config, dict))
+def configure_logging():
+    logging_class_path = conf.get("core", "logging_config_class")


This fails when not present (exception)

Good point, thanks

bolkedebruin · 2017-09-27T21:05:15Z

airflow/logging_config.py

-            'Unable to load custom logging, using default config instead'
-        )
-        from airflow.config_templates.default_airflow_local_settings import \
+    if logging_class_path == '':


As the above fails this is a bit ugly

bolkedebruin · 2017-09-27T21:06:05Z

airflow/utils/module_loading.py

+        module_path, class_name = dotted_path.rsplit('.', 1)
+    except ValueError as err:
+        raise ImportError(
+            '{} doesn\'t look like a module path'.format(dotted_path)


No need to escape if using “”

bolkedebruin · 2017-09-27T21:06:52Z

airflow/utils/module_loading.py

+import logging
+from importlib import import_module
+
+


You are missing the tests for this function ;-)

I'll wait 😁

Fokko · 2017-09-29T11:21:01Z

Took a while. I've extended the tests with a more complicated path etc.airflow.config.custom_airflow_local_settings.LOGGING_CONFIG. This caused the tests to fail because the temporary path that I was creating required those __init__.py files for Python 2.7.

ahvigil · 2017-09-29T19:01:35Z

my thinking on this was that logging_config_path could point to a json file that could be json.loaded, and the resulting dict could be passed to logging.config.dictConfig. Its a bit annoying and not entirely necessary requiring an importable object on the python path (supporting logging_config_path=/path/to/my/config.json would be nice), but maybe this can be a follow up issue.

bolkedebruin · 2017-09-30T06:40:15Z

@Fokko almost LGTM. Can you make clear in updating.md (if then wise) what one needs to do when currently using the S3 / GCS log? Ie. what do I need to do to make it work on 1.9.0. Then we can merge.

Fokko · 2017-10-01T11:53:27Z

I've updated the UPDATING.md, please let me know if it is clear. My technical writing is not one of my qualities. Also rebased against master.

ashb · 2017-10-01T12:29:51Z

UPDATING.md

+
+IF you are explicitly setting the `REMOTE_BASE_LOG_FOLDER`, you will need to:
+- Create a logging configuration `airflow/confi_templates/airflow_logging_settings.py` and configure the remote paths.
+- Place it on the Python path.


Place it in a directory inside the Python import path, ensuring that any __init__.py files exist so that it is importable.

ashb · 2017-10-01T12:30:56Z

One minor point to add to the updating docs, and I think it should be called out explicitly in UPDATING.md that to use S3Log or GCSLogs you will need a custom logging config -- otherwise people would miss it unless they checked the comment in the default template which they might not.

Fokko · 2017-10-01T12:51:23Z

@ashb Thanks, I've made it more explicit.

Fokko · 2017-10-01T18:48:47Z

@ashb Can you do a suggestion?

Change the configuration of the logging to make use of the python logging and make the configuration easy configurable. Some of the settings which are now not needed anymore since they can easily be implemented in the config file.

Change the configuration of the logging to make use of the python logging and make the configuration easy configurable. Some of the settings which are now not needed anymore since they can easily be implemented in the config file. Closes #2631 from Fokko/AIRFLOW-1611-customize- logging-in-airflow (cherry picked from commit 3c3a65a) Signed-off-by: Bolke de Bruin <bolke@xs4all.nl>

Change the configuration of the logging to make use of the python logging and make the configuration easy configurable. Some of the settings which are now not needed anymore since they can easily be implemented in the config file. Closes apache#2631 from Fokko/AIRFLOW-1611-customize- logging-in-airflow

Fokko changed the title ~~[AIRFLOW-1611] Customize logging~~ [AIRFLOW-1611] Customize logging config Sep 26, 2017

ashb reviewed Sep 26, 2017

View reviewed changes

bolkedebruin reviewed Sep 27, 2017

View reviewed changes

Fokko force-pushed the AIRFLOW-1611-customize-logging-in-airflow branch 9 times, most recently from bd0e003 to 8193c7c Compare September 29, 2017 11:18

ahvigil mentioned this pull request Sep 29, 2017

[AIRFLOW-1457] fix user-defined log import #2648

Closed

4 tasks

Fokko force-pushed the AIRFLOW-1611-customize-logging-in-airflow branch from 8193c7c to 494e5c6 Compare October 1, 2017 11:52

Fokko force-pushed the AIRFLOW-1611-customize-logging-in-airflow branch from 494e5c6 to 5bcc170 Compare October 1, 2017 12:06

ashb reviewed Oct 1, 2017

View reviewed changes

Fokko force-pushed the AIRFLOW-1611-customize-logging-in-airflow branch from 5bcc170 to e99f80c Compare October 1, 2017 12:50

[AIRFLOW-1611] Customize logging

d71aff1

Change the configuration of the logging to make use of the python logging and make the configuration easy configurable. Some of the settings which are now not needed anymore since they can easily be implemented in the config file.

Fokko force-pushed the AIRFLOW-1611-customize-logging-in-airflow branch from e99f80c to d71aff1 Compare October 2, 2017 11:27

asfgit closed this in 3c3a65a Oct 2, 2017


		Airflow's logging has been rewritten to uses Python’s builtin `logging` module to perform logging of the application. By extending classes with the existing `LoggingMixin`, all the logging will go through a central logger. The main benefit is easier configuration of the logging by setting a single python file. Disclaimer; there is still some inline configuration, but this will be removed eventually.

		The loggers and handlers can be adjusted by placing a python file on the filsystem, and point the `logging_config_path` setting in your `airflow.cfg` to the configuration file. Please note that the variable that contains the config needs to be names `LOGGING_CONFIG`:

[AIRFLOW-1611] Customize logging config #2631

[AIRFLOW-1611] Customize logging config #2631

Conversation

Fokko commented Sep 25, 2017 • edited

JIRA

Tests

Commits

codecov-io commented Sep 25, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko Sep 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashb commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko commented Sep 29, 2017

ahvigil commented Sep 29, 2017

bolkedebruin commented Sep 30, 2017 • edited

Fokko commented Oct 1, 2017

Choose a reason for hiding this comment

ashb commented Oct 1, 2017

Fokko commented Oct 1, 2017

Fokko commented Oct 1, 2017

Fokko commented Sep 25, 2017 •

edited

codecov-io commented Sep 25, 2017 •

edited

Fokko Sep 26, 2017 •

edited

bolkedebruin commented Sep 30, 2017 •

edited