Event logger config takes instance instead of class (#7997)

* allow preconfigured event logger instance; deprecate specifying class

* add func docs and simplify conditions

* modify docs to reflect EVENT_LOGGER cfg change

* commit black formatting fixes and license header

* add type checking, fix other pre-commit failues

* remove superfluous/wordy condition

* fix flake8 failure

* fix new black failure

* dedent warning msg; use f-strings

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

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():

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

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())