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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
