Skip to content
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.

Sign up for GitHub

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

chore: add logs to db test connection and post failures #13223

Closed
wants to merge 19 commits into from
Closed

chore: add logs to db test connection and post failures #13223

Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a6f9c78
add logs to db test connection and post failures
hughhhh Feb 18, 2021
d59e224
yerp
hughhhh Feb 19, 2021
07c3fb3
fix the logs
hughhhh Feb 19, 2021
9a84f7e
remove
hughhhh Feb 19, 2021
df6d508
change the logs
hughhhh Feb 19, 2021
732a638
add more information
hughhhh Feb 22, 2021
0bf3b38
fix this
hughhhh Feb 22, 2021
dc1d2c4
add info for engine
hughhhh Feb 22, 2021
94e17d3
fix merge conflicts
hughhhh Feb 22, 2021
9fa0f73
remove unneeded code
hughhhh Feb 22, 2021
99c5eca
Merge branch 'master' of https://github.com/apache/incubator-superset…
hughhhh Feb 22, 2021
65dadfd
move stats to commands
hughhhh Feb 23, 2021
12c9acc
add stats for engine
hughhhh Feb 23, 2021
eb697ec
clean up stat reference
hughhhh Feb 24, 2021
23d9411
just to see the results of this test
hughhhh Feb 24, 2021
0af5cab
working test
hughhhh Mar 1, 2021
23a7a6f
another working test
hughhhh Mar 1, 2021
42ae1cd
add db api exception
hughhhh Mar 1, 2021
d323eb1
cleanup
hughhhh Mar 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions superset/databases/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ def post(self) -> Response:
except DatabaseInvalidError as ex:
return self.response_422(message=ex.normalized_messages())
except DatabaseConnectionFailedError as ex:
logger.warning("Database connection failed: %s", item["sqlalchemy_uri"])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log will be masked uri

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that item["sqlalchemy_uri" is only updated with the masked version if line 236 is reached - if CreateDatabaseCommand(...).run() fails, as we can expect if this code is reached, then I believe there's a good chance that the full sqlalchemy_uri will be logged.

While the masked URI is better than the full URI, it still exposes username and additional connection information that it might be better not to have in logs.

return self.response_422(message=str(ex))
except DatabaseCreateFailedError as ex:
logger.error(
Expand Down Expand Up @@ -607,6 +608,7 @@ def test_connection( # pylint: disable=too-many-return-statements
TestConnectionDatabaseCommand(g.user, item).run()
return self.response(200, message="OK")
except DatabaseTestConnectionFailedError as ex:
logger.warning(ex)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this log will read out the following:
WARNING:superset.databases.api:Could not load database driver: mssql+pymssql

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is one possible error message - others are not so innocuous and include username and password information. We've seen that with a few different database engines.

return self.response_422(message=str(ex))

@expose("/<int:pk>/related_objects/", methods=["GET"])
Expand Down