-
Notifications
You must be signed in to change notification settings - Fork 17.9k
feat: add event_logger to test_connection and create_database commands #13468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 31 commits
9e2f527
6730835
80cd5bd
f4c8a5f
e2ab146
440a9d6
1266d12
d7e1247
b130962
9718486
2d05a37
f9d5f3f
58fcd28
47fe377
2fda45e
35763ef
31abdc8
12e7731
e0871f0
bcf85f8
f42efe2
82612ea
9d7fc0e
bfa5477
68f25d0
f483764
538d9dd
ea81349
46ce47f
f4eaac1
8f6d379
cfbab0c
55601ee
cffe48e
c4039f9
b2c120c
2f86e2a
3856999
33e4180
67d42b4
f11ab8a
7c28363
ce0240a
e20f6dd
864173d
a904cd8
46095ca
c48ecd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ | |
| ) | ||
| from superset.databases.commands.test_connection import TestConnectionDatabaseCommand | ||
| from superset.databases.dao import DatabaseDAO | ||
| from superset.extensions import db, security_manager | ||
| from superset.extensions import db, event_logger, security_manager | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
@@ -50,9 +50,13 @@ def run(self) -> Model: | |
|
|
||
| try: | ||
| TestConnectionDatabaseCommand(self._actor, self._properties).run() | ||
| except Exception: | ||
| db.session.rollback() | ||
| raise DatabaseConnectionFailedError() | ||
| except Exception: # pylint: disable=broad-except | ||
| with event_logger( | ||
| action="db_connection_failed", | ||
| engine=database.db_engine_spec.__name__, | ||
| ): | ||
| db.session.rollback() | ||
| raise DatabaseConnectionFailedError() | ||
|
|
||
| # adding a new database we always want to force refresh schema list | ||
| schemas = database.get_all_schema_names(cache=False) | ||
|
|
@@ -63,8 +67,11 @@ 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( | ||
| action=f"db_creation_failed.{ex.exception}", | ||
|
betodealmeida marked this conversation as resolved.
Outdated
|
||
| engine=database.db_engine_spec.__name__, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hughhhh what did you want to do about making the engine string consistent around each of these logs so that we can group by the same engine later when observing the logs?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ended up changing all the engine references in the |
||
| ): | ||
| raise DatabaseCreateFailedError() | ||
| return database | ||
|
|
||
| def validate(self) -> None: | ||
|
|
@@ -84,4 +91,5 @@ def validate(self) -> None: | |
| if exceptions: | ||
| exception = DatabaseInvalidError() | ||
| exception.add_list(exceptions) | ||
| raise exception | ||
| with event_logger(action="db_connection_failed"): | ||
| raise exception | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ | |
| ) | ||
| from superset.databases.dao import DatabaseDAO | ||
| from superset.exceptions import SupersetSecurityException | ||
| from superset.extensions import event_logger | ||
| from superset.models.core import Database | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -55,25 +56,49 @@ def run(self) -> None: | |
| impersonate_user=self._properties.get("impersonate_user", False), | ||
| encrypted_extra=self._properties.get("encrypted_extra", "{}"), | ||
| ) | ||
| if database is not None: | ||
| database.set_sqlalchemy_uri(uri) | ||
| database.db_engine_spec.mutate_db_for_connection_test(database) | ||
| username = self._actor.username if self._actor is not None else None | ||
| engine = database.get_sqla_engine(user_name=username) | ||
| if database is None: | ||
| raise DBAPIError("Database is not found", None, None) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can actually remove this and fix the type annotation in |
||
|
|
||
| database.set_sqlalchemy_uri(uri) | ||
| database.db_engine_spec.mutate_db_for_connection_test(database) | ||
| username = self._actor.username if self._actor is not None else None | ||
| engine = database.get_sqla_engine(user_name=username) | ||
| with closing(engine.raw_connection()) as conn: | ||
| if not engine.dialect.do_ping(conn): | ||
| raise DBAPIError(None, None, None) | ||
| except (NoSuchModuleError, ModuleNotFoundError): | ||
|
|
||
| with event_logger( | ||
| action="test_connection_success", engine=make_url(uri).drivername | ||
| ): | ||
| pass | ||
|
|
||
| except (NoSuchModuleError, ModuleNotFoundError) as ex: | ||
| driver_name = make_url(uri).drivername | ||
| raise DatabaseTestConnectionDriverError( | ||
| message=_("Could not load database driver: {}").format(driver_name), | ||
| ) | ||
| except DBAPIError: | ||
| raise DatabaseTestConnectionFailedError() | ||
| with event_logger( | ||
| action=f"test_connection_error.{ex.__class__.__name__}", | ||
| engine=make_url(uri).drivername, | ||
| ): | ||
| raise DatabaseTestConnectionDriverError( | ||
| message=_("Could not load database driver: {}").format(driver_name), | ||
| ) | ||
| except DBAPIError as ex: | ||
| with event_logger( | ||
| action=f"test_connection_error.{ex.__class__.__name__}", | ||
| engine=make_url(uri).drivername, | ||
| ): | ||
| raise DatabaseTestConnectionFailedError() | ||
| except SupersetSecurityException as ex: | ||
| raise DatabaseSecurityUnsafeError(message=str(ex)) | ||
| except Exception: | ||
| raise DatabaseTestConnectionUnexpectedError() | ||
| with event_logger( | ||
| action=f"test_connection_error.{ex.__class__.__name__}", | ||
| engine=make_url(uri).drivername, | ||
| ): | ||
| raise DatabaseSecurityUnsafeError(message=str(ex)) | ||
| except Exception as ex: # pylint: disable=broad-except | ||
| with event_logger( | ||
| action=f"test_connection_error.{ex.__class__.__name__}", | ||
| engine=make_url(uri).drivername, | ||
| ): | ||
| raise DatabaseTestConnectionUnexpectedError() | ||
|
|
||
| def validate(self) -> None: | ||
| database_name = self._properties.get("database_name") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For places where we do a rollback we should it outside the context manager, otherwise if the logging fails the transaction won't be rolled back:
Reversing the order ensures that even if the logging fails the rollback will still go through.