Log errors caught during sync steps #26306
Merged
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
noahmoss
added
the
backport
noahmoss
force-pushed
the
nm-logging-for-sync-step-exceptions
branch
from
9a97114
to
0951752
Compare
paoliniluis
added
backport
Codecov ReportBase: 64.28% // Head: 64.29% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #26306 +/- ##
==========================================
+ Coverage 64.28% 64.29% +0.01%
==========================================
Files 3148 3149 +1
Lines 92202 92257 +55
Branches 11699 11704 +5
==========================================
+ Hits 59270 59315 +45
- Misses 28242 28254 +12
+ Partials 4690 4688 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
github-actions bot
pushed a commit
that referenced
this pull request
Nov 10, 2022
* log errors caught during sync steps * remove accidental extra parens
calherries
pushed a commit
that referenced
this pull request
Nov 10, 2022
* log errors caught during sync steps * remove accidental extra parens
calherries
added a commit
that referenced
this pull request
Nov 16, 2022
* Initial support for pg for `date-diff` * Make the useful-dates closer to one day * Add simple FE stuff * shorter test bodies * Ensure we can use datediff functions in arithmetic expressions * Correctly disable datediff for redshift * simplify var names * Support week * cleanup test * :datediff -> :datetimediff * ngoc's suggestions * Better acceptance test for datetimediff * sort ns * embrace the different cases for results * bigquery day month year * Reverse args * Update test * Centralize tests * Change postgres day, month, year behaviour * Refactor keep identity * Tidy tests * Tidy * Fix bigquery week * Add week tests * Fix bigquery week * Change mysql day, month, year behaviour * Add test for hour, minute, second * Fix postgres hour minute second * Formatting * Fix bigquery hour, minute, second * Formatting * Fix postgres timestamptz * WIP * Allow literals in datediff clauses * Uncomment tests * Fix bigquery when reporting timezone is not UTC * Linting * Moving away from dataset based tests * Add timezone tests for week and tidy * Remove unused import * Consolidate tests * Remove with-time-column dataset * Remove more-useful-dates defdataset * Remove redshift driver WIP * Typo * Move DatetimeLiteral clause into DateTimeExpressionArg * Try changing test order * Remove mt/with-report-timezone-id nil * Add year report timezone tests * Rename * Rename * Remove unused tables from useful-dates * Remove useful-dates * Update helper-text-strings * Tidy * Swap order of mt/with-report-timezone-id * Change with-report-timezone-id; notify databases after running test * Use temp setting for report-timezone instead * Update helper-test-strings * Handle literals in `datetimediff-base-base` * Update src/metabase/driver/postgres.clj Co-authored-by: metamben <103100869+metamben@users.noreply.github.com> * Simplify postgres second * Tidy: prefer hx arithmetic functions * Drop coercion for string timestamp args for now * Extract helper * Revert "Change with-report-timezone-id; notify databases after running test" This reverts commit 7abb543. * Use ->timestamptz * Use hx/->timestamp * Rename dataset * Undo changes to datetime-arithmetics? as these will not match by default * Use proper format for offset datetime literals * Shorten datetime literals in tests * Removing notify-all-databases-updated from report-timezone setting * Fix datetime-arithmetics test * Revert "Fix datetime-arithmetics test" This reverts commit 9141582. * Revert "Undo changes to datetime-arithmetics? as these will not match by default" This reverts commit 9cb05f5. * Add UTC timezone to tests * Coerce strings to datetimes for ISO formats * Revert "Removing notify-all-databases-updated from report-timezone setting" This reverts commit 3735643. * Add comment to postgres driver implementation * Formatting * Disable datediff from redshift for the moment (for ever?) * Override redshift driver/database-supports? * Fix comments mixed up by refactoring * Fix comments mixed up by refactoring 2 * Tidy comment * Rename datetimediff to datetime-diff/datetimeDiff * date-add -> datetime-add * Linting * add datetime-subtract to `datetime-arithmetics?` rework tests a bit as well. * Update docstring * Use ->temporal-type and trunc to handle report-timezone for bigquery * Log errors caught during sync steps (#26306) * log errors caught during sync steps * remove accidental extra parens * Errors combining datetime interval addition with datetime functions (#26279) * Add failing tests * Fix failing tests * Update shared/src/metabase/mbql/util.cljc Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> * datetime-add and datetime subtract should annotate type by col type * Fix infer-expression-type for datetime-add/subtract with second, minute, hour * Undo last commit; they actually always return :type/DateTime * Fix test based on last commit * Undo unrelated refactor * Only test drivers that support expressions * Only test drivers that support expressions, again * Update tests from legacy mbql * Change infered-col-type to be a function again, not macro * Fix test Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> * whitespace * Remove comments * Add explanation for datetime_diff * Refactor: replace cast and add `mt/with-driver :bigquery-cloud-sdk` where report-timezone is relevant * Add failing tests * Fix failing tests * Technically it should be bigquery-type * Update modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> * Move documentation to metabase.mbql.schema * Update DatetimeDiffUnits Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> * Add error type and optimize case expressions to driver implementations * refactor for brevity * Fix error with postgres * Fix error with postgres * Handle string literal parsing in wrap-value-literals * Remove ->timestamptz * Add comments + TODOs showing arithmetic expressions should return numeric values * Add explanation of arithmetic expression as docstring * Add test for normalize-mbql-clause-tokens * Remove unused import * Remove unused form * Switch tests to use attempted-murders dataset * Undo optimization that broke tests * Validate non-temporal types for bigquery * Add error handling for incorrect types * Fix mysql type checking * Fix mysql type checking * invalid-parameter -> invalid-query * Use date-trunc and extract * Remove unit error handling * DatetimeLiteral -> DateOrDatetimeLiteral * Remove unused binding * Fix mysql type checking Co-authored-by: Callum Herries <hi@callumherries.com> Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com> Co-authored-by: metamben <103100869+metamben@users.noreply.github.com> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>
nemanjaglumac
pushed a commit
that referenced
this pull request
Nov 16, 2022
* Initial support for pg for `date-diff` * Make the useful-dates closer to one day * Add simple FE stuff * shorter test bodies * Ensure we can use datediff functions in arithmetic expressions * Correctly disable datediff for redshift * simplify var names * Support week * cleanup test * :datediff -> :datetimediff * ngoc's suggestions * Better acceptance test for datetimediff * sort ns * embrace the different cases for results * bigquery day month year * Reverse args * Update test * Centralize tests * Change postgres day, month, year behaviour * Refactor keep identity * Tidy tests * Tidy * Fix bigquery week * Add week tests * Fix bigquery week * Change mysql day, month, year behaviour * Add test for hour, minute, second * Fix postgres hour minute second * Formatting * Fix bigquery hour, minute, second * Formatting * Fix postgres timestamptz * WIP * Allow literals in datediff clauses * Uncomment tests * Fix bigquery when reporting timezone is not UTC * Linting * Moving away from dataset based tests * Add timezone tests for week and tidy * Remove unused import * Consolidate tests * Remove with-time-column dataset * Remove more-useful-dates defdataset * Remove redshift driver WIP * Typo * Move DatetimeLiteral clause into DateTimeExpressionArg * Try changing test order * Remove mt/with-report-timezone-id nil * Add year report timezone tests * Rename * Rename * Remove unused tables from useful-dates * Remove useful-dates * Update helper-text-strings * Tidy * Swap order of mt/with-report-timezone-id * Change with-report-timezone-id; notify databases after running test * Use temp setting for report-timezone instead * Update helper-test-strings * Handle literals in `datetimediff-base-base` * Update src/metabase/driver/postgres.clj Co-authored-by: metamben <103100869+metamben@users.noreply.github.com> * Simplify postgres second * Tidy: prefer hx arithmetic functions * Drop coercion for string timestamp args for now * Extract helper * Revert "Change with-report-timezone-id; notify databases after running test" This reverts commit 7abb543. * Use ->timestamptz * Use hx/->timestamp * Rename dataset * Undo changes to datetime-arithmetics? as these will not match by default * Use proper format for offset datetime literals * Shorten datetime literals in tests * Removing notify-all-databases-updated from report-timezone setting * Fix datetime-arithmetics test * Revert "Fix datetime-arithmetics test" This reverts commit 9141582. * Revert "Undo changes to datetime-arithmetics? as these will not match by default" This reverts commit 9cb05f5. * Add UTC timezone to tests * Coerce strings to datetimes for ISO formats * Revert "Removing notify-all-databases-updated from report-timezone setting" This reverts commit 3735643. * Add comment to postgres driver implementation * Formatting * Disable datediff from redshift for the moment (for ever?) * Override redshift driver/database-supports? * Fix comments mixed up by refactoring * Fix comments mixed up by refactoring 2 * Tidy comment * Rename datetimediff to datetime-diff/datetimeDiff * date-add -> datetime-add * Linting * add datetime-subtract to `datetime-arithmetics?` rework tests a bit as well. * Update docstring * Use ->temporal-type and trunc to handle report-timezone for bigquery * Log errors caught during sync steps (#26306) * log errors caught during sync steps * remove accidental extra parens * Errors combining datetime interval addition with datetime functions (#26279) * Add failing tests * Fix failing tests * Update shared/src/metabase/mbql/util.cljc Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> * datetime-add and datetime subtract should annotate type by col type * Fix infer-expression-type for datetime-add/subtract with second, minute, hour * Undo last commit; they actually always return :type/DateTime * Fix test based on last commit * Undo unrelated refactor * Only test drivers that support expressions * Only test drivers that support expressions, again * Update tests from legacy mbql * Change infered-col-type to be a function again, not macro * Fix test Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> * whitespace * Remove comments * Add explanation for datetime_diff * Refactor: replace cast and add `mt/with-driver :bigquery-cloud-sdk` where report-timezone is relevant * Add failing tests * Fix failing tests * Technically it should be bigquery-type * Update modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> * Move documentation to metabase.mbql.schema * Update DatetimeDiffUnits Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> * Add error type and optimize case expressions to driver implementations * refactor for brevity * Fix error with postgres * Fix error with postgres * Handle string literal parsing in wrap-value-literals * Remove ->timestamptz * Add comments + TODOs showing arithmetic expressions should return numeric values * Add explanation of arithmetic expression as docstring * Add test for normalize-mbql-clause-tokens * Remove unused import * Remove unused form * Switch tests to use attempted-murders dataset * Undo optimization that broke tests * Validate non-temporal types for bigquery * Add error handling for incorrect types * Fix mysql type checking * Fix mysql type checking * invalid-parameter -> invalid-query * Use date-trunc and extract * Remove unit error handling * DatetimeLiteral -> DateOrDatetimeLiteral * Remove unused binding * Fix mysql type checking Co-authored-by: Callum Herries <hi@callumherries.com> Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com> Co-authored-by: metamben <103100869+metamben@users.noreply.github.com> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>
metabase-bot bot
added a commit
that referenced
this pull request
Nov 17, 2022
* Initial support for pg for `date-diff` * Make the useful-dates closer to one day * Add simple FE stuff * shorter test bodies * Ensure we can use datediff functions in arithmetic expressions * Correctly disable datediff for redshift * simplify var names * Support week * cleanup test * :datediff -> :datetimediff * ngoc's suggestions * Better acceptance test for datetimediff * sort ns * embrace the different cases for results * bigquery day month year * Reverse args * Update test * Centralize tests * Change postgres day, month, year behaviour * Refactor keep identity * Tidy tests * Tidy * Fix bigquery week * Add week tests * Fix bigquery week * Change mysql day, month, year behaviour * Add test for hour, minute, second * Fix postgres hour minute second * Formatting * Fix bigquery hour, minute, second * Formatting * Fix postgres timestamptz * WIP * Allow literals in datediff clauses * Uncomment tests * Fix bigquery when reporting timezone is not UTC * Linting * Moving away from dataset based tests * Add timezone tests for week and tidy * Remove unused import * Consolidate tests * Remove with-time-column dataset * Remove more-useful-dates defdataset * Remove redshift driver WIP * Typo * Move DatetimeLiteral clause into DateTimeExpressionArg * Try changing test order * Remove mt/with-report-timezone-id nil * Add year report timezone tests * Rename * Rename * Remove unused tables from useful-dates * Remove useful-dates * Update helper-text-strings * Tidy * Swap order of mt/with-report-timezone-id * Change with-report-timezone-id; notify databases after running test * Use temp setting for report-timezone instead * Update helper-test-strings * Handle literals in `datetimediff-base-base` * Update src/metabase/driver/postgres.clj Co-authored-by: metamben <103100869+metamben@users.noreply.github.com> * Simplify postgres second * Tidy: prefer hx arithmetic functions * Drop coercion for string timestamp args for now * Extract helper * Revert "Change with-report-timezone-id; notify databases after running test" This reverts commit 7abb543. * Use ->timestamptz * Use hx/->timestamp * Rename dataset * Undo changes to datetime-arithmetics? as these will not match by default * Use proper format for offset datetime literals * Shorten datetime literals in tests * Removing notify-all-databases-updated from report-timezone setting * Fix datetime-arithmetics test * Revert "Fix datetime-arithmetics test" This reverts commit 9141582. * Revert "Undo changes to datetime-arithmetics? as these will not match by default" This reverts commit 9cb05f5. * Add UTC timezone to tests * Coerce strings to datetimes for ISO formats * Revert "Removing notify-all-databases-updated from report-timezone setting" This reverts commit 3735643. * Add comment to postgres driver implementation * Formatting * Disable datediff from redshift for the moment (for ever?) * Override redshift driver/database-supports? * Fix comments mixed up by refactoring * Fix comments mixed up by refactoring 2 * Tidy comment * Rename datetimediff to datetime-diff/datetimeDiff * date-add -> datetime-add * Linting * add datetime-subtract to `datetime-arithmetics?` rework tests a bit as well. * Update docstring * Use ->temporal-type and trunc to handle report-timezone for bigquery * Log errors caught during sync steps (#26306) * log errors caught during sync steps * remove accidental extra parens * Errors combining datetime interval addition with datetime functions (#26279) * Add failing tests * Fix failing tests * Update shared/src/metabase/mbql/util.cljc Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> * datetime-add and datetime subtract should annotate type by col type * Fix infer-expression-type for datetime-add/subtract with second, minute, hour * Undo last commit; they actually always return :type/DateTime * Fix test based on last commit * Undo unrelated refactor * Only test drivers that support expressions * Only test drivers that support expressions, again * Update tests from legacy mbql * Change infered-col-type to be a function again, not macro * Fix test Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> * whitespace * Remove comments * Add explanation for datetime_diff * Refactor: replace cast and add `mt/with-driver :bigquery-cloud-sdk` where report-timezone is relevant * Add failing tests * Fix failing tests * Technically it should be bigquery-type * Update modules/drivers/bigquery-cloud-sdk/src/metabase/driver/bigquery_cloud_sdk.clj Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> * Move documentation to metabase.mbql.schema * Update DatetimeDiffUnits Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> * Add error type and optimize case expressions to driver implementations * refactor for brevity * Fix error with postgres * Fix error with postgres * Handle string literal parsing in wrap-value-literals * Remove ->timestamptz * Add comments + TODOs showing arithmetic expressions should return numeric values * Add explanation of arithmetic expression as docstring * Add test for normalize-mbql-clause-tokens * Remove unused import * Remove unused form * Switch tests to use attempted-murders dataset * Undo optimization that broke tests * Validate non-temporal types for bigquery * Add error handling for incorrect types * Fix mysql type checking * Fix mysql type checking * invalid-parameter -> invalid-query * Use date-trunc and extract * Remove unit error handling * DatetimeLiteral -> DateOrDatetimeLiteral * Remove unused binding * Fix mysql type checking Co-authored-by: Callum Herries <hi@callumherries.com> Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com> Co-authored-by: metamben <103100869+metamben@users.noreply.github.com> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com> Co-authored-by: dpsutton <dan@dpsutton.com> Co-authored-by: Callum Herries <hi@callumherries.com> Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com> Co-authored-by: metamben <103100869+metamben@users.noreply.github.com> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com> Co-authored-by: Cam Saul <1455846+camsaul@users.noreply.github.com>
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.
Currently, when exceptions are thrown during sync steps, they're caught within
run-step-with-metadataand stuffed into the run metadata in the:throwablefield. This is put into thetask_historytable in the app db but doesn't show up in the logs at all, which makes issues like #26268 pretty tricky to detect and diagnose if you're not aware of that table. This PR just adds a log when exceptions are caught here.I'll note that there's already similar error handling & logging in do-sync-operation which wraps the entire sync-metadata job. But it never sees errors that are thrown within individual steps, because they're intercepted by
run-step-with-metadata. This is worth keeping sincedo-sync-operationis used for other sync jobs as well, but perhaps this is the reason that logging here was overlooked.