superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From b...@apache.org
Subject [incubator-superset] branch master updated: Event logger config takes instance instead of class (#7997)
Date Thu, 08 Aug 2019 20:47:25 GMT
This is an automated email from the ASF dual-hosted git repository.

beto pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git


The following commit(s) were added to refs/heads/master by this push:
     new 9233a63  Event logger config takes instance instead of class (#7997)
9233a63 is described below

commit 9233a63a16311a7fe2012e97a949b9b3fd6434a1
Author: Dave Smith <dave.a.smith@gmail.com>
AuthorDate: Thu Aug 8 13:47:18 2019 -0700

    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       |  4 ++--
 superset/__init__.py        |  6 ++++--
 superset/utils/log.py       | 43 +++++++++++++++++++++++++++++++++++++++++++
 tests/event_logger_tests.py | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/docs/installation.rst b/docs/installation.rst
index a012ab9..e560657 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 12d8cf6..3b503a4 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 94cdfb4..768e6b3 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 0000000..a7a1f22
--- /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())


Mime
View raw message