Fix assorted source freshness edgecases so check is run or actionable information #9825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some nodes, like SourceDefinition nodes, don't have a `build_path` property. This is problematic because we take in nodes with no type checking, and assume they have properties sometimes, like `build_path`. This was just the case in BaseRunner's `_handle_generic_exception` and `_handle_interal_exception` methods. Thus to stop dbt from crashing when trying to handle an exception related to a node without a `build_path`, we added an private method to the BaseRunner class for safely trying to get `build_path`.
Previously we were passing arguments during the `Note` event instantiations in freshness.py as positional arguments. This would cause not the desired `Note` event to be emitted, but instead get the message ``` [Note] Don't use positional arguments when constructing logging events ``` which was our fault, not the users'. Additionally, we were passing the level for the event in the `Note` instantiation when we needed to be passing it to the `fire_event` method.
… possible Previously if a source freshness check didn't have a `loaded_at_field` and metadata source freshness wasn't supported by the adapter, then we'd log a warning message and let the source freshness check continue. This was problematic because the source freshness check couldn't actually continue and the process would raise an error in the form ``` type object argument after ** must be a mapping, not NoneType ``` because the `freshness` variable was never getting set. This error wasn't particularly helpful for any person running into it. So instead of letting that error happen we now deliberately raise an error with helpful information.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9825 +/- ##
==========================================
+ Coverage 88.06% 88.10% +0.03%
==========================================
Files 178 178
Lines 22495 22461 -34
==========================================
- Hits 19810 19789 -21
+ Misses 2685 2672 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…error This test directly tests that when a source freshness check doesn't have a `loaded_at_field` and the adapter in use doesn't support metadata checks, then the appropriate error message gets raised. That is, it directly tests the change made in a162d53. This test indirectly tests the changes in both 7ec2f82 and 7b0ff31 as the appropriate error can only be raised because we've fixed other upstream issues via those commits.
QMalcolm
force-pushed
the
qmalcolm--9078-fix-source-freshness-error-handling
branch
from
7d6f64e
to
1abcaa4
Compare
MichelleArk
approved these changes
Mar 27, 2024
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.7.latest 1.7.latest
# Navigate to the new working tree
cd .worktrees/backport-1.7.latest
# Create a new branch
git switch --create backport-9825-to-1.7.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a029661e237357087823cf8052a1970bed9e39cd
# Push it to GitHub
git push --set-upstream origin backport-9825-to-1.7.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.7.latestThen, create a pull request where the |
QMalcolm
added a commit
that referenced
this pull request
Mar 27, 2024
… information (#9825) * Ensure BaseRunner handles nodes without `build_path` Some nodes, like SourceDefinition nodes, don't have a `build_path` property. This is problematic because we take in nodes with no type checking, and assume they have properties sometimes, like `build_path`. This was just the case in BaseRunner's `_handle_generic_exception` and `_handle_interal_exception` methods. Thus to stop dbt from crashing when trying to handle an exception related to a node without a `build_path`, we added an private method to the BaseRunner class for safely trying to get `build_path`. * Use keyword arguments when instantiating `Note` events in freshness.py Previously we were passing arguments during the `Note` event instantiations in freshness.py as positional arguments. This would cause not the desired `Note` event to be emitted, but instead get the message ``` [Note] Don't use positional arguments when constructing logging events ``` which was our fault, not the users'. Additionally, we were passing the level for the event in the `Note` instantiation when we needed to be passing it to the `fire_event` method. * Raise error when `loaded_at_field` is `None` and metadata check isn't possible Previously if a source freshness check didn't have a `loaded_at_field` and metadata source freshness wasn't supported by the adapter, then we'd log a warning message and let the source freshness check continue. This was problematic because the source freshness check couldn't actually continue and the process would raise an error in the form ``` type object argument after ** must be a mapping, not NoneType ``` because the `freshness` variable was never getting set. This error wasn't particularly helpful for any person running into it. So instead of letting that error happen we now deliberately raise an error with helpful information. * Add test which ensures bad source freshness checks raise appropriate error This test directly tests that when a source freshness check doesn't have a `loaded_at_field` and the adapter in use doesn't support metadata checks, then the appropriate error message gets raised. That is, it directly tests the change made in a162d53. This test indirectly tests the changes in both 7ec2f82 and 7b0ff31 as the appropriate error can only be raised because we've fixed other upstream issues via those commits. * Add changelog entry for source freshness edgecase fixes
QMalcolm
added a commit
that referenced
this pull request
Mar 27, 2024
… information (#9825) * Ensure BaseRunner handles nodes without `build_path` Some nodes, like SourceDefinition nodes, don't have a `build_path` property. This is problematic because we take in nodes with no type checking, and assume they have properties sometimes, like `build_path`. This was just the case in BaseRunner's `_handle_generic_exception` and `_handle_interal_exception` methods. Thus to stop dbt from crashing when trying to handle an exception related to a node without a `build_path`, we added an private method to the BaseRunner class for safely trying to get `build_path`. * Use keyword arguments when instantiating `Note` events in freshness.py Previously we were passing arguments during the `Note` event instantiations in freshness.py as positional arguments. This would cause not the desired `Note` event to be emitted, but instead get the message ``` [Note] Don't use positional arguments when constructing logging events ``` which was our fault, not the users'. Additionally, we were passing the level for the event in the `Note` instantiation when we needed to be passing it to the `fire_event` method. * Raise error when `loaded_at_field` is `None` and metadata check isn't possible Previously if a source freshness check didn't have a `loaded_at_field` and metadata source freshness wasn't supported by the adapter, then we'd log a warning message and let the source freshness check continue. This was problematic because the source freshness check couldn't actually continue and the process would raise an error in the form ``` type object argument after ** must be a mapping, not NoneType ``` because the `freshness` variable was never getting set. This error wasn't particularly helpful for any person running into it. So instead of letting that error happen we now deliberately raise an error with helpful information. * Add test which ensures bad source freshness checks raise appropriate error This test directly tests that when a source freshness check doesn't have a `loaded_at_field` and the adapter in use doesn't support metadata checks, then the appropriate error message gets raised. That is, it directly tests the change made in a162d53. This test indirectly tests the changes in both 7ec2f82 and 7b0ff31 as the appropriate error can only be raised because we've fixed other upstream issues via those commits. * Add changelog entry for source freshness edgecase fixes
QMalcolm
added a commit
that referenced
this pull request
Mar 29, 2024
…heck is run or actionable information (#9825) (#9826) * Fix assorted source freshness edgecases so check is run or actionable information (#9825) * Ensure BaseRunner handles nodes without `build_path` Some nodes, like SourceDefinition nodes, don't have a `build_path` property. This is problematic because we take in nodes with no type checking, and assume they have properties sometimes, like `build_path`. This was just the case in BaseRunner's `_handle_generic_exception` and `_handle_interal_exception` methods. Thus to stop dbt from crashing when trying to handle an exception related to a node without a `build_path`, we added an private method to the BaseRunner class for safely trying to get `build_path`. * Use keyword arguments when instantiating `Note` events in freshness.py Previously we were passing arguments during the `Note` event instantiations in freshness.py as positional arguments. This would cause not the desired `Note` event to be emitted, but instead get the message ``` [Note] Don't use positional arguments when constructing logging events ``` which was our fault, not the users'. Additionally, we were passing the level for the event in the `Note` instantiation when we needed to be passing it to the `fire_event` method. * Raise error when `loaded_at_field` is `None` and metadata check isn't possible Previously if a source freshness check didn't have a `loaded_at_field` and metadata source freshness wasn't supported by the adapter, then we'd log a warning message and let the source freshness check continue. This was problematic because the source freshness check couldn't actually continue and the process would raise an error in the form ``` type object argument after ** must be a mapping, not NoneType ``` because the `freshness` variable was never getting set. This error wasn't particularly helpful for any person running into it. So instead of letting that error happen we now deliberately raise an error with helpful information. * Add test which ensures bad source freshness checks raise appropriate error This test directly tests that when a source freshness check doesn't have a `loaded_at_field` and the adapter in use doesn't support metadata checks, then the appropriate error message gets raised. That is, it directly tests the change made in a162d53. This test indirectly tests the changes in both 7ec2f82 and 7b0ff31 as the appropriate error can only be raised because we've fixed other upstream issues via those commits. * Add changelog entry for source freshness edgecase fixes * Update imports of dbt.artifacts to pre-dbt.artifacts locations Moving things to dbt.artifacts started to happen AFTER the release of dbt-core 1.7, thus 1.7 has no concept of dbt.articacts. The changes brought in via the previous commit, were cherry-picked from main (1.8.0 beta) and thus reference dbt.artifacts. This caused everything to blow up, because dbt.artifacts doesn't exist. Here we've updated the dbt.artifacts imports to their pre-dbt.artifacts paths.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #9078
Problem
We were hitting multiple edge cases in the source freshness task which were raising unhelpful error messages.
Solution
BaseTaskto gracefully handle nodes withoutbuild_pathNoteevents use keyword paramsloaded_at_fieldAND the project adapter doesn't support metadata checksChecklist