dbt-labs / dbt-core

dbt enables data analysts and engineers to transform their data using the same practices that software engineers use to build applications.
https://getdbt.com
Apache License 2.0
9.99k stars 1.63k forks source link

[CT-2836] extend fire_event to handle warn-error and warn-error-options for WarnLevel exceptions, deprecate warn_or_error #8133

Open MichelleArk opened 1 year ago

MichelleArk commented 1 year ago

It's pretty easy to accidentally call fire_event directly for WarnLevel exceptions, rather than warn_or_error. This will cause unexpected behaviour because the --warn-error or --warn-error-options flag handling won't apply. For example: https://github.com/dbt-labs/dbt-core/issues/8130

Potential Solution(s)

Update fire_event to accept an optional argument that indicates whether the method was called from warn_or_error. If a WarnLevel exception is not called from warn_or_error, raise a DbtInternalError to indicate the issue. This could be dangerous to turn on for any warnings that are not covered by unit / integration tests as end users can start hitting this error when they were simply firing events in the past.

An alternative could be to write a test that ensures any WarnLevel exceptions are always called from warn_or_error rather than fire_event directly. This would be less risky than validating the calls during execution, but I'm not sure how we'd go about writing a test like that.

Any other ideas?

gshank commented 1 year ago

Just wanted to point out that initially there was no expectation that all WarnLevel exceptions would be handled by warn_or_error. There were specific exceptions that were wrapped in that function, and when we implemented the much nicer functionality to specify which of the events wrapped. in warn_or_error would throw errors that was continued. I have no objection to making this more consistent, just wanted to point out that it wasn't a bug that some warnings were not included.

MichelleArk commented 1 year ago

Makes sense. Thank you, that's helpful historical context

MichelleArk commented 1 year ago

From technical refinement: instead of trying to enforce that fire_event is always called from warn_or_error for WarnLevel events, let's get rid of (or safely deprecate, if there adapter-related considerations) warn_or_error in favor of bringing the--warn-error and --warn-error-options handling for WarnLevel inputs into fire_event itself.

This may cause some warnings that were previously not elevated to errors when the global --warn-error flag was set, but arguably these would just be a collection of bugs similar to #8133 that should still be remedied. The --warn-error-options config could be used to selectively exclude some warnings from being elevated to errors if this is a concern for upgrading.