[GitHub] [superset] betodealmeida commented on a change in pull request #13346: feat: db connection analytics

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

betodealmeida commented on a change in pull request #13346:
URL: https://github.com/apache/superset/pull/13346#discussion_r583805293



##########
File path: superset/databases/commands/create.py
##########
@@ -69,8 +64,9 @@ def run(self) -> Model:
             security_manager.add_permission_view_menu("database_access", database.perm)
             db.session.commit()
         except DAOCreateFailedError as ex:
-            logger.exception(ex.exception)
-            raise DatabaseCreateFailedError()
+            with event_logger.log_context(action=f"db_creation_failed.{ex.exception}"):

Review comment:
       One problem is that adding the flag `enrich_with_request_context` would be a breaking change (our event logger, for example, would have to be updated).
   
   I think the best way here is:
   
   1. Create a new method in `AbstractEventLogger` called `log_with_context`, that does all the job that `log_context` does but is a function, not a context manager. The method calls `log` at the end, with all the extracted context.
   2. Change `AbstractEventLogger.log_context` to use `log_with_context` for the enrichment, removing the shared code between them.
   3. In this PR, call the new `log_with_context`. Since it delegates the actual logging to `log` it will work with all existing event loggers, and is not a breaking change..
   
   Eventually I think we should have the event logger itself be a context manager by implementing `__enter__` and `__exit__` methods, so we could call:
   
   ```python
   with event_logger(action="foo"):
      do_something()
   ```
   
   Since we can do this by adding methods to `AbstractEventLogger` it would also not be a breaking change.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org