feat: add event_logger to test_connection and create_database commands

[hughhhh]

SUMMARY

Leverage refactored event_logger with context management

Dependent on #13441

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[betodealmeida]

I left a few comments and mentioned a couple nits, but I have one concern with the rollback() that's being called inside the event_logger context manager. We should move it out and call it before logging, to ensure that it goes through even if logging fails.

[betodealmeida]

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:

Suggested change

	with event_logger(
	action="db_connection_failed",
	engine=database.db_engine_spec.__name__,
	):
	db.session.rollback()
	raise DatabaseConnectionFailedError()
	db.session.rollback()
	event_logger.log_with_context(
	action="db_connection_failed",
	engine=database.db_engine_spec.__name__,
	)
	raise DatabaseConnectionFailedError()

Reversing the order ensures that even if the logging fails the rollback will still go through.

[betodealmeida]

Total nit, but in places where we just raise an exception inside the context manager like here and in lines 94-97 I think it would make more sense to use the log_with_context function instead:

Suggested change

	with event_logger(
	action=f"db_creation_failed.{ex.__class__.__name__}",
	engine=database.db_engine_spec.__name__,
	):
	raise DatabaseCreateFailedError()
	event_logger.log_with_context(
	action=f"db_creation_failed.{ex.__class__.__name__}",
	engine=database.db_engine_spec.__name__,
	)
	raise DatabaseCreateFailedError()

[betodealmeida]

We can actually remove this and fix the type annotation in DatabaseDAO.build_db_for_connection_test, the class method never returns None.

[betodealmeida]

Update docstring here and in the other 2 tests 😄

[betodealmeida]

Now that we're using pytest, it's better to run tests using plain asserts instead, since it gives more context when the test fails. Also, we can check that the exception raised has the right error message:

Suggested change

	with self.assertRaises(DatabaseTestConnectionUnexpectedError):
	command_without_db_name.run()
	with pytest.raises(DatabaseTestConnectionUnexpectedError) as excinfo:
	command_without_db_name.run()
	assert str(excinfo.value) == (
	"Unexpected error occurred, please check your logs for details"
	)

Same in the other 2 tests below.

[betodealmeida]

[eschutho]

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

[hughhhh]

I ended up changing all the engine references in the event_logger.log_with_context to pull data from the database.db_engine_spec.__name__ I say we go with this for now and see the data then see if we need to create some mapping either in the service or downstream