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

Clean up run results #2943

Merged
merged 9 commits into from
Dec 15, 2020
Merged

Conversation

kwigley
Copy link
Contributor

@kwigley kwigley commented Dec 10, 2020

resolves #2493

Description

This branches attempts to rationalize run result status handling.

  • The resulting status of a run will be stored in a status field and related info in the message field. this is a breaking change
    Example:
    • success status -> status returned from database adapter stored in message
    • error status -> error message stored in the message field
  • Instead of including the entire node obj in run results, we just write the unique_id. this is a breaking change
  • Renames source freshness results state field to status. this is a breaking change

New schema can be viewed here: https://schemas.getdbt.com/dbt/run-results/v1.json this is definitely slimmer than the previous run results schema.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR (I updated existing tests)
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Dec 10, 2020
@kwigley kwigley force-pushed the feature/refactor-run-results branch 2 times, most recently from 0d99a63 to ebdb5fd Compare December 11, 2020 18:44
Comment on lines +109 to +111
"run_status": str(run_model_result.status).upper(),
"run_skipped": run_model_result.status == NodeStatus.Skipped,
"run_error": run_model_result.status == NodeStatus.Error,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will affect model run event tracking data, I'm not sure if there is anything else to consider here.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for calling it out! we'll just need to account for this on our end

Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

Lots of changes! Looks good. Nice and clean.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This is looking really solid! The new run_results.json feels both more sensible and more concise. There are some small things I want to improve in the future (not this PR), mostly around a more structured version of the message (a la #2580 or #2747). Let's discuss #2747 this afternoon.

Three small things I've noticed in the CLI output:

  1. When running a model that fails, the skip looks great, but the errant model itself reports the message instead of the status:
13:36:52 | 1 of 2 START view model dbt_jcohen.model_a........................... [RUN]
13:36:52 | 1 of 2 ERROR creating view model dbt_jcohen.model_a.................. [Database Error in model model_a (models/model_a.sql)
  syntax error at or near ";"
  LINE 7: ;aoushfs;dulfhsdlf
          ^
  compiled SQL at target/run/testy/models/model_a.sql in 0.12s]

I believe the same occurs with seeds as well.

  1. When a test has a fail or warn status, the stdout "summary" now reports the statuses as their message:
Failure in test unique_model_a_col3 (models/whatever.yml)
  Status: fail

  compiled SQL at target/compiled/testy/models/whatever.yml/schema_test/unique_model_a_col3.sql

Warning in test unique_model_b_col3 (models/whatever.yml)
  Status: warn

  compiled SQL at target/compiled/testy/models/whatever.yml/schema_test/unique_model_b_col3.sql

Whereas previously it wrapped the message like so:

Failure in test unique_model_a_col3 (models/whatever.yml)
  Got 1 result, expected 0.

  compiled SQL at target/compiled/testy/models/whatever.yml/schema_test/unique_model_a_col3.sql

Warning in test unique_model_b_col3 (models/whatever.yml)
  Got 1 result, expected 0.

  compiled SQL at target/compiled/testy/models/whatever.yml/schema_test/unique_model_b_col3.sql

I'm already hoping to change that message output to be a bit more descriptive (and customizable!) per our proposed test changes in v0.20.0, so I'd prefer to keep it consistent for now.

  1. When snapshotting source freshness, if there is a database error (i.e. malformed SQL syntax), we should surface that error in the stdout "summary". (This is distinct from the error produced when a query completes successfully and finds stale sources.)

This PR:

14:52:45 | 1 of 1 ERROR freshness of dbt_jcohen.my_seed......................... [ERROR in 0.07s]
14:52:46 | Done.

Current behavior (v0.18.1):

14:54:27 | 1 of 1 START freshness of dbt_jcohen.my_seed......................... [RUN]
14:54:27 | 1 of 1 ERROR freshness of dbt_jcohen.my_seed......................... [ERROR in 0.04s]

Database Error in source my_seed (models/whatever.yml)
  Expected a timestamp value when querying field 'col3' of table "jerco"."dbt_jcohen"."my_seed" but received value of type 'str' instead
14:54:27 | Done.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

I did a quick tour of seed, run, test, snapshot, source snapshot-freshness, and docs generate. All CLI output + run results are looking good. Nice work!

@kwigley kwigley marked this pull request as ready for review December 15, 2020 16:14
@kwigley kwigley merged commit 454ddc6 into dev/kiyoshi-kuromiya Dec 15, 2020
@kwigley kwigley deleted the feature/refactor-run-results branch December 15, 2020 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run results reporting is a mess
3 participants