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.

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

Fix handling of top-level exceptions #5560

Merged
merged 4 commits into from
Jul 28, 2022
Merged

Fix handling of top-level exceptions #5560

merged 4 commits into from
Jul 28, 2022

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Jul 26, 2022

resolves #5564

Description

Errors that were caught as BaseExceptions in the main function were not displaying or storing the stack trace because it was flagged as debug level and also flagged as not to be written to the file. I have removed the "NoFile" flag from the events that handle top level exceptions and left it only for integration tests and "clean" command events.

A different pr will be used to address catching exception in threads.

Checklist

@gshank gshank requested a review from a team July 26, 2022 15:32
@gshank gshank requested review from a team as code owners July 26, 2022 15:32
@cla-bot cla-bot bot added the cla:yes label Jul 26, 2022
@gshank gshank marked this pull request as draft July 26, 2022 15:32
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@@ -142,7 +142,6 @@ def main(args=None):
exit_code = e.code

except BaseException as e:
traceback.print_exc()
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this was an incidental inclusion in v1.2.0, I'd also be in favor of a PR that removed this line alone, and backporting that PR to 1.2.latest for inclusion in a v1.2.1 patch release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually looking at two different problems, really, the lack of a stack trace for top level exceptions and catching exceptions in threads. I could make the current code the "lack of a stack trace" fix and backport it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

good plan.

@gshank gshank marked this pull request as ready for review July 27, 2022 13:33
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

Looks good to me!
No blocker but do we want to make this available under a flag so engineers/users can access more trace when they need to?

@ChenyuLInx ChenyuLInx mentioned this pull request Jul 28, 2022
5 tasks
@gshank gshank merged commit b43fc76 into main Jul 28, 2022
@gshank gshank deleted the ct-462-capture_exceptions branch July 28, 2022 03:52
@gshank gshank added the backport 1.2.latest This PR will be backported to the 1.2.latest branch label Jul 28, 2022
github-actions bot pushed a commit that referenced this pull request Jul 28, 2022
gshank added a commit that referenced this pull request Jul 28, 2022
(cherry picked from commit b43fc76)

Co-authored-by: Gerda Shank <gerda@dbtlabs.com>
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.2.latest This PR will be backported to the 1.2.latest branch cla:yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-943] Captured top-level exception do not store into log or display stacktrace.
4 participants