[bug] Fix error when attempting to log errors with --log-format json #207
Conversation
| # 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) |
There was a problem hiding this comment.
does this need to happen within the if BQ_QUERY_JOB_SPLIT in exc_message: block?
| logger.debug(exc_message) | |
| logger.debug(exc_message) |
There was a problem hiding this comment.
@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!
|
I'm going to close this PR, insofar as we'd like to try resolving the root cause in |
resolves #206
Description
Instead of attempting to log the bare Exception object, log the stringified version of it. This will have no impact on the default logging behavior, while allowing this message to be json serializable.
The contributing guide mentions using
changiefor changelog entries, but it doesn't seem like that's set up for this subrepo, let me know if I'm missing something!Checklist
CHANGELOG.mdand added information about my change to the "dbt-bigquery next" section.