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

[bug] Fix error when attempting to log errors with --log-format json #207

Closed
wants to merge 2 commits into from
Closed

[bug] Fix error when attempting to log errors with --log-format json #207

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f09398c
[bug] Fix error when attempting to log errors with --log-format json
OwenKephart Jun 28, 2022
0f7fca3
fix exc_message logging placement
OwenKephart Jun 28, 2022
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
5 changes: 3 additions & 2 deletions dbt/adapters/bigquery/connections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,19 @@ def exception_handler(self, sql):
raise RuntimeException(message)

except Exception as e:
exc_message = str(e)
logger.debug("Unhandled error while running:\n{}".format(sql))
logger.debug(e)
if isinstance(e, RuntimeException):
logger.debug(exc_message)
# during a sql query, an internal to dbt exception was raised.
# this sounds a lot like a signal handler and probably has
# useful information, so raise it without modification.
raise
exc_message = str(e)
# the google bigquery library likes to add the query log, which we
# don't want to log. Hopefully they never change this!
if BQ_QUERY_JOB_SPLIT in exc_message:
exc_message = exc_message.split(BQ_QUERY_JOB_SPLIT)[0].strip()
logger.debug(exc_message)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to happen within the if BQ_QUERY_JOB_SPLIT in exc_message: block?

Suggested change
logger.debug(exc_message)
logger.debug(exc_message)

Copy link
Author

Choose a reason for hiding this comment

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

@dataders I don't have strong feelings here, but in the original implementation, the error message would be logged in all scenarios (as logger.debug(e) was outside of all conditionals). In the proposed version, the error message would not be logged in the scenario that the Exception was not a RuntimeException, and BQ_QUERY_JOB_SPLIT was not in exc_message.

I might have over-indexed on the comment saying "...likes to add the query log, which we don't want to log". I took that to mean that it would be incorrect to log the query log stuff even in the debug logs (so I only logged the exc_message once that had been filtered out). If that stuff is ok to make it into debug-level logs, then I think the best approach is to log the entire exc_message (pre-split) at the top of this function.

Happy to go in whichever direction with this though!

raise RuntimeException(exc_message)

def cancel_open(self) -> None:
Expand Down