Skip to content

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

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

Closed
hughhhh wants to merge 19 commits into from
Closed

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

Changes from 1 commit
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.exception("Database connection failed")

@willbarrett willbarrett Feb 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we downgrade these from exception logging? Connection failures are an expected part of setting up new database connections and shouldn't wind up in exception tracking systems.

@willbarrett willbarrett Feb 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want to understand the reasons for failures, it would also be a good idea to log the exception. However, beware: these exceptions frequently contain database credentials, which it would be bad to track.

@betodealmeida betodealmeida Feb 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good point on the level. Just a note, @willbarrett: @logger.exception logs the full traceback plus the message, so it would've helped figuring out the reason.

@nytai nytai Feb 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

logger.warning("Database connection failed", exc_info=True) also achieves the same. As the one currently running the error/exception logs portion of our biweekly alerts review, I would greatly appreciate if we reduced the amount of expected exceptions being logged at error and exception level. It would make my job significantly easier, as well as help reduce the strain on our log quota

@dpgaspar dpgaspar Feb 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with @willbarrett concern, unfortunately some engines dbapi surface exceptions with sensitive information logger.warning("Database connection failed", exc_info=True) will actually log this info. we can lower it to debug, since on normal situations production systems are not set to this level.

Also there's an ongoing effort to remap these exceptions at the engine spec level #12869, so that we can add more granularity to connection failures for example

@willbarrett willbarrett Feb 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm now wondering if there's a better way to get the info that @betodealmeida and @hughhhh are looking for - would you be able to describe the questions that you're trying to answer with this logging? Then we can support you better in coming to a safe solution.

@hughhhh hughhhh Feb 19, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We want to answer the following:
What db engines are failing the most?
What are the test connection and db connection exception error for these db engines?

If we can get this information with other data let us know

@willbarrett willbarrett Feb 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What DB engines are failing the most?
I think you could parse the submitted SQLAlchemy URL to pull out the scheme portion of the URL and log that on failure. This would avoid the risk of capturing secure credentials.

What are the the test connection and DB connection exceptions for these engines?
Would it be sufficient to log the exception class from the engine without the message? The class will give you the category of error without including any of the potentially sensitive details. If this would work, you could more easily perform this logging from the command rather than in the endpoint.

@nytai nytai Feb 20, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a great use case for statsd!

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.exception("Database test connection failed")
return self.response_422(message=str(ex))

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