diff --git a/docs/installation.rst b/docs/installation.rst index a012ab9da..e5606574f 100644 --- a/docs/installation.rst +++ b/docs/installation.rst @@ -738,9 +738,9 @@ Example of a simple JSON to Stdout class:: print(json.dumps(log)) -Then on Superset's config reference the class you want to use:: +Then on Superset's config pass an instance of the logger type you want to use. - EVENT_LOGGER = JSONStdOutEventLogger + EVENT_LOGGER = JSONStdOutEventLogger() Upgrading diff --git a/superset/__init__.py b/superset/__init__.py index 12d8cf611..3b503a48c 100644 --- a/superset/__init__.py +++ b/superset/__init__.py @@ -36,7 +36,7 @@ from superset import config from superset.connectors.connector_registry import ConnectorRegistry from superset.security import SupersetSecurityManager from superset.utils.core import pessimistic_connection_handling, setup_cache -from superset.utils.log import DBEventLogger +from superset.utils.log import DBEventLogger, get_event_logger_from_cfg_value wtforms_json.init() @@ -220,7 +220,9 @@ _feature_flags = app.config.get("DEFAULT_FEATURE_FLAGS") or {} _feature_flags.update(app.config.get("FEATURE_FLAGS") or {}) # Event Logger -event_logger = app.config.get("EVENT_LOGGER", DBEventLogger)() +event_logger = get_event_logger_from_cfg_value( + app.config.get("EVENT_LOGGER", DBEventLogger()) +) def get_feature_flags(): diff --git a/superset/utils/log.py b/superset/utils/log.py index 94cdfb470..768e6b303 100644 --- a/superset/utils/log.py +++ b/superset/utils/log.py @@ -18,7 +18,11 @@ from abc import ABC, abstractmethod from datetime import datetime import functools +import inspect import json +import logging +import textwrap +from typing import Any, cast, Type from flask import current_app, g, request @@ -83,6 +87,45 @@ class AbstractEventLogger(ABC): return current_app.config.get("STATS_LOGGER") +def get_event_logger_from_cfg_value(cfg_value: object) -> AbstractEventLogger: + """ + This function implements the deprecation of assignment of class objects to EVENT_LOGGER + configuration, and validates type of configured loggers. + + The motivation for this method is to gracefully deprecate the ability to configure + EVENT_LOGGER with a class type, in favor of preconfigured instances which may have + required construction-time injection of proprietary or locally-defined dependencies. + + :param cfg_value: The configured EVENT_LOGGER value to be validated + :return: if cfg_value is a class type, will return a new instance created using a + default con + """ + result: Any = cfg_value + if inspect.isclass(cfg_value): + logging.warning( + textwrap.dedent( + """ + In superset private config, EVENT_LOGGER has been assigned a class object. In order to + accomodate pre-configured instances without a default constructor, assignment of a class + is deprecated and may no longer work at some point in the future. Please assign an object + instance of a type that implements superset.utils.log.AbstractEventLogger. + """ + ) + ) + + event_logger_type = cast(Type, cfg_value) + result = event_logger_type() + + # Verify that we have a valid logger impl + if not isinstance(result, AbstractEventLogger): + raise TypeError( + "EVENT_LOGGER must be configured with a concrete instance of superset.utils.log.AbstractEventLogger." + ) + + logging.info(f"Configured event logger of type {type(result)}") + return cast(AbstractEventLogger, result) + + class DBEventLogger(AbstractEventLogger): def log(self, user_id, action, *args, **kwargs): from superset.models.core import Log diff --git a/tests/event_logger_tests.py b/tests/event_logger_tests.py new file mode 100644 index 000000000..a7a1f222a --- /dev/null +++ b/tests/event_logger_tests.py @@ -0,0 +1,44 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import logging +import unittest + +from superset.utils.log import DBEventLogger, get_event_logger_from_cfg_value + + +class TestEventLogger(unittest.TestCase): + def test_returns_configured_object_if_correct(self): + # test that assignment of concrete AbstractBaseClass impl returns unmodified object + obj = DBEventLogger() + res = get_event_logger_from_cfg_value(obj) + self.assertTrue(obj is res) + + def test_event_logger_config_class_deprecation(self): + # test that assignment of a class object to EVENT_LOGGER is correctly deprecated + res = None + + # print warning if a class is assigned to EVENT_LOGGER + with self.assertLogs(level="WARNING"): + res = get_event_logger_from_cfg_value(DBEventLogger) + + # class is instantiated and returned + self.assertIsInstance(res, DBEventLogger) + + def test_raises_typerror_if_not_abc_impl(self): + # test that assignment of non AbstractEventLogger derived type raises TypeError + with self.assertRaises(TypeError): + get_event_logger_from_cfg_value(logging.getLogger())