Behavior change cumulative type param #10909

DevonFulcher · 2024-10-23T15:44:49Z

[Edited after my last commit]

Resolves #10960
Linear issue: SL-2935
docs: dbt-labs/docs.getdbt.com#6393

Problem & Solution

The type params for cumulative metrics have moved, so a behavior change needs to be implemented to surface this for users. This is what the warning looks like:

15:36:22  [WARNING]: Cumulative fields `type_params.window` and
`type_params.grain_to_date` have been moved and will soon be deprecated. Please
nest those values under `type_params.cumulative_type_params.window` and
`type_params.cumulative_type_params.grain_to_date`. See documentation on
behavior changes:
https://docs.getdbt.com/reference/global-configs/behavior-changes.

Here is what the error looks like when the user flips the behavior flag:

21:39:18  Cumulative fields `type_params.window` and `type_params.grain_to_date` should be nested under `type_params.cumulative_type_params.window` and `type_params.cumulative_type_params.grain_to_date`. Invalid metrics: orders_last_7_days. See documentation on behavior changes: https://docs.getdbt.com/reference/global-configs/behavior-changes.

Testing:

I ran dbt parse in jaffle-sl-template and I saw the warning message due to this metric defined there:

    type: cumulative
    type_params:
      measure: order_count
      window: 7 days

I updated this metric and didn't see the deprecation warning:

    type: cumulative
    type_params:
      measure:
        name: order_count
      cumulative_type_params:
        window: 7 days

I changed the metric back to the deprecated version, added this to dbt_project.yaml, and saw the error message and that dbt parse failed:

flags:
  require_nested_cumulative_type_params: True

Checklist

I have read the contributing guide and understand what's expected of me.
I have run this code in development, and it appears to resolve the stated issue.
This PR includes tests, or tests are not required or relevant for this PR.
This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
This PR includes type annotations for new and modified functions.

codecov · 2024-10-23T15:51:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.06%. Comparing base (8a17a0d) to head (d6f6283).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10909      +/-   ##
==========================================
- Coverage   89.07%   89.06%   -0.01%     
==========================================
  Files         183      183              
  Lines       23550    23580      +30     
==========================================
+ Hits        20976    21002      +26     
- Misses       2574     2578       +4

Flag	Coverage Δ
integration	`86.36% <92.30%> (-0.02%)`	⬇️
unit	`62.79% <100.00%> (+0.51%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
Unit Tests	`62.79% <100.00%> (+0.51%)`	⬆️
Integration Tests	`86.36% <92.30%> (-0.02%)`	⬇️

ChenyuLInx

This change looks good to me except for one behavior. Also thanks for enumerate the tests you did in the PR description!

I would expect when you set require_nested_cumulative_type_params, the warning from using unnested param would be an error. Since now user is required to set that param in nested fashion

DevonFulcher · 2024-10-31T19:01:35Z

This change looks good to me except for one behavior. Also thanks for enumerate the tests you did in the PR description!

I would expect when you set require_nested_cumulative_type_params, the warning from using unnested param would be an error. Since now user is required to set that param in nested fashion

@ChenyuLInx So, this follows other behavior changes like here by using a warning message, not an error message. Since the legacy behavior is still supported, I think it makes sense to keep as a warning. The message is to notify to the user to upgrade their legacy yaml spec, not to indicate that there was a failure during invocation.

ChenyuLInx · 2024-10-31T21:31:27Z

The case there is if you change the flag to true, the warning will be gone, but dbt will actually behave differently(change the macro overwrite order). The changing behavior part is an important piece of this flag.
I didn't quite follow will the behavior of the SL command change if the user flip the flag to true but has old definition. Happy to sync live really quick

ChenyuLInx · 2024-11-04T23:10:12Z

We talked on Slack, and @DevonFulcher is going to change the behavior to failure if the schema is not adjusted but the flag is flipped to true.

courtneyholcomb · 2024-11-05T21:19:56Z

core/dbt/contracts/graph/semantic_manifest.py

+                            ),
+                            message=f"Cumulative metric {m} fields `type_params.window` and `type_params.grain_to_date` should be nested under `type_params.cumulative_type_params.window` and `type_params.cumulative_type_params.grain_to_date`. See documentation on behavior changes: https://docs.getdbt.com/reference/global-configs/behavior-changes.",
+                        )
+                        for m in metrics_using_old_params


Can we update this to be just one error that lists all the metrics with deprecated fields? We previously had complaints from customers about this because it was extremely noisy to have a warning for each metric.

courtneyholcomb

One suggestion but otherwise LGTM! Thank you!

courtneyholcomb · 2024-11-05T22:14:37Z

core/dbt/events/types.py

+    def code(self) -> str:
+        return "D019"
+
+    def message(self) -> str:


Would it be easy to add the names of the outdated metrics in this message? Otherwise it might be hard for users to find.
But if that doesn't work with the behavior change objects easily, it probably isn't worth it.

Ya, I don't think that is a straightforward change, given that it would change the interface. I think that could be an enhancement in the future.

cla-bot bot added the cla:yes label Oct 23, 2024

DevonFulcher closed this Oct 23, 2024

DevonFulcher force-pushed the behavior_change_cumulative_type_param branch from c6ac260 to 6da0bc6 Compare October 23, 2024 15:55

DevonFulcher reopened this Oct 23, 2024

Base automatically changed from Behavior_change_for_mf_timespine_without_yaml_configuration to main October 31, 2024 15:40

DevonFulcher force-pushed the behavior_change_cumulative_type_param branch from 2071399 to 92b6954 Compare October 31, 2024 15:47

Behavior change for mf timespinse without yaml config

02d1902

DevonFulcher force-pushed the behavior_change_cumulative_type_param branch from 113c63a to 02d1902 Compare October 31, 2024 15:49

DevonFulcher marked this pull request as ready for review October 31, 2024 15:55

DevonFulcher requested a review from a team as a code owner October 31, 2024 15:55

DevonFulcher requested a review from courtneyholcomb October 31, 2024 15:55

github-actions bot added the community This PR is from a community member label Oct 31, 2024

ChenyuLInx reviewed Oct 31, 2024

View reviewed changes

ChenyuLInx approved these changes Nov 4, 2024

View reviewed changes

DevonFulcher added 2 commits November 5, 2024 13:29

Flipping behavior flag causes parse error

61969a9

Added more tests

af097c1

courtneyholcomb reviewed Nov 5, 2024

View reviewed changes

Appending just one error

d6f6283

courtneyholcomb approved these changes Nov 5, 2024

View reviewed changes

ChenyuLInx merged commit e26af57 into main Nov 5, 2024
60 checks passed

ChenyuLInx deleted the behavior_change_cumulative_type_param branch November 5, 2024 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Behavior change cumulative type param #10909

Behavior change cumulative type param #10909

DevonFulcher commented Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading

ChenyuLInx left a comment •

edited

Loading

DevonFulcher commented Oct 31, 2024

ChenyuLInx commented Oct 31, 2024

ChenyuLInx commented Nov 4, 2024

courtneyholcomb Nov 5, 2024

courtneyholcomb left a comment

courtneyholcomb Nov 5, 2024

DevonFulcher Nov 5, 2024

Behavior change cumulative type param #10909

Behavior change cumulative type param #10909

Conversation

DevonFulcher commented Oct 23, 2024 • edited Loading

Problem & Solution

Checklist

codecov bot commented Oct 23, 2024 • edited Loading

Codecov Report

ChenyuLInx left a comment • edited Loading

Choose a reason for hiding this comment

DevonFulcher commented Oct 31, 2024

ChenyuLInx commented Oct 31, 2024

ChenyuLInx commented Nov 4, 2024

courtneyholcomb Nov 5, 2024

Choose a reason for hiding this comment

courtneyholcomb left a comment

Choose a reason for hiding this comment

courtneyholcomb Nov 5, 2024

Choose a reason for hiding this comment

DevonFulcher Nov 5, 2024

Choose a reason for hiding this comment

DevonFulcher commented Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 23, 2024 •

edited

Loading

ChenyuLInx left a comment •

edited

Loading