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

[CT-1148] [Bug] Follow-up to metrics field renaming #5807

Closed
2 tasks done
jtcohen6 opened this issue Sep 12, 2022 · 1 comment · Fixed by #5825
Closed
2 tasks done

[CT-1148] [Bug] Follow-up to metrics field renaming #5807

jtcohen6 opened this issue Sep 12, 2022 · 1 comment · Fixed by #5825
Assignees
Labels
bug Something isn't working semantic Issues related to the semantic layer
Milestone

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Sep 12, 2022

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

The field renaming in #5775 means that a user upgrading from v1.2 → v1.3 will see a parsing error immediately after upgrading, if they have metrics already defined in their project:

$ dbt parse
dbt.exceptions.ParsingException: Parsing Error
  Invalid metrics config given in FilePath(searched_path='models', relative_path='metric.yml', modification_time=1662969785.677178, project_root='/Users/jerco/dev/scratch/testy') @ metrics: {'name': 'new_customers', 'label': 'New Customers', 'model': "ref('dim_customers')", 'description': 'The number of paid customers using the product', 'type': 'count', 'sql': 'user_id', 'timestamp': 'signup_date', 'time_grains': ['day', 'week', 'month'], 'dimensions': ['plan', 'country'], 'filters': [{'field': 'is_paying', 'operator': 'is', 'value': 'true'}, {'field': 'lifetime_value', 'operator': '>=', 'value': '100'}, {'field': 'company_name', 'operator': '!=', 'value': "'Acme, Inc'"}, {'field': 'signup_date', 'operator': '>=', 'value': "'2020-01-01'"}], 'meta': {'team': 'Finance'}} - at path []: 'calculation_method' is a required property

This also breaks backwards compatibility for users of "Slim CI":

$ dbt ls -s state:modified --state state/
08:07:02  Encountered an error:
Field "metrics" of type Mapping[str, ParsedMetric] in WritableManifest has invalid value {'metric.my_project.new_customers': {'fqn': ['my_project', 'new_customers'], 'unique_id': 'metric.my_project.new_customers', 'package_name': 'my_project', 'root_path': '/Users/jerco/dev/scratch/testy', 'path': 'metric.yml', 'original_file_path': 'models/metric.yml', 'name': 'new_customers', 'description': 'The number of paid customers using the product', 'label': 'New Customers', 'type': 'count', 'sql': 'user_id', 'timestamp': 'signup_date', 'filters': [{'field': 'is_paying', 'operator': 'is', 'value': 'true'}, {'field': 'lifetime_value', 'operator': '>=', 'value': '100'}, {'field': 'company_name', 'operator': '!=', 'value': "'Acme, Inc'"}, {'field': 'signup_date', 'operator': '>=', 'value': "'2020-01-01'"}], 'time_grains': ['day', 'week', 'month'], 'dimensions': ['plan', 'country'], 'model': "ref('dim_customers')", 'model_unique_id': None, 'resource_type': 'metric', 'meta': {'team': 'Finance'}, 'tags': [], 'sources': [], 'depends_on': {'macros': [], 'nodes': ['model.my_project.dim_customers']}, 'refs': [['dim_customers']], 'metrics': [], 'created_at': 1662969788.598332}}
08:07:02  Traceback (most recent call last):
  File "<string>", line 73, in from_dict
  File "<string>", line 73, in <dictcomp>
  File "<string>", line 76, in from_dict
mashumaro.exceptions.MissingField: Field "calculation_method" of type str is missing in ParsedMetric instance

Metrics are still an officially experimental feature, so we have reserved the right to do this — but we should aim for a smoother upgrade experience if possible.

Expected Behavior

  • Upgrading raises a deprecation warning, but does not immediately break. Specifically, if I have a metric definition containing attributes named sql and type, or type: expression, dbt should implicitly rename those and store those attributes with the new names/values instead.
  • Compatibility for --state comparison to older manifests, if possible. Could we extend the logic in upgrade_manifest_json to that effect? Otherwise, we should declare that v1.3 breaks forwards compatibility for manifests from v1.0-1.2.

Steps To Reproduce

  1. Create a metric as documented
  2. Using dbt-core==1.2, generate a manifest (e.g. via dbt ls), and save it somewhere locally (mv target/manifest.json state/)
  3. Upgrade to dbt-core==1.3
  4. Run dbt parse
  5. Run dbt ls -s state:modified --state ... (where ... is relative path to the previous manifest)

Relevant log output

No response

Environment

- OS: macOS 12.4
- Python: 3.9.13
- dbt: 1.2 -> 1.3

Which database adapter are you using with dbt?

No response

Additional Context

No response

@jtcohen6 jtcohen6 added bug Something isn't working metrics labels Sep 12, 2022
@jtcohen6 jtcohen6 added this to the v1.3 milestone Sep 12, 2022
@github-actions github-actions bot changed the title [Bug] Follow-up to metrics field renaming [CT-1148] [Bug] Follow-up to metrics field renaming Sep 12, 2022
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Sep 12, 2022

What this might want to look like (quick & naive version): af4d10a

Testing:

  • Metrics with old attribute names, also type: expressioncalculation_method: derived
  • We do have a functional test that checks for back/forward compatibility when using older manifests in Slim CI. We don't have a metric in that test project right now — perhaps we should! — would just require recreating the manifests for older versions (v4 through v7)
    # This is a *very* simple project, with just one model in it.
    models__my_model_sql = """
    select 1 as id
    """

@callum-mcdata callum-mcdata self-assigned this Sep 12, 2022
@jtcohen6 jtcohen6 self-assigned this Sep 13, 2022
@jtcohen6 jtcohen6 removed their assignment Nov 14, 2022
@jtcohen6 jtcohen6 added the semantic Issues related to the semantic layer label Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semantic Issues related to the semantic layer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants