[Bug] Metric arguments need to be renamed

[fivetran-joemarkiewicz]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the issue

In the latest release (v1.1.0) a warning is present indicating the metric arguments within the ad_reporting_metrics.yml are out of date and going to be deprecated. As such, we should update the metric definitions to be in line with the current dbt-metrics standards.

See below for a relevant warning message:

Relevant error log or model output

17:21:35  [WARNING]: Deprecated functionality
dbt-core v1.3 renamed attributes for metrics:
  'sql'              -> 'expression'
  'type'             -> 'calculation_method'
  'type: expression' -> 'calculation_method: derived'
The old metric parameter names will be fully deprecated in v1.4.
Please remove them from the metric definition of metric 'cost_per_click'
Relevant issue here: https://github.com/dbt-labs/dbt-core/issues/5849

Expected behavior

No warning is displayed when users run the ad reporting package.

dbt Project configurations

N/A

Package versions

v1.1.0

What database are you using dbt with?

postgres, redshift, snowflake, bigquery, databricks

dbt Version

v1.3.1

Additional Context

This could be a great first issue for any community member wanting to try to contribute to the package offering!

Are you willing to open a PR to help address this issue?

• [ ] Yes.
• [ ] Yes, but I will need assistance and will schedule time during our office hours for guidance
• [ ] No.

[fivetran-joemarkiewicz]

@callum-mcdata, I wanted to tag you in real quick to help with this issue - do you know if adjusting these arguments would resulting in a breaking change for the package users?

I imagine since we have a required dbt range of [">=1.3.0", "<2.0.0"] it should not cause any breaking runs for existing users. Is that in line with your understanding? Or would the renaming of these metrics cause failures?

[callum-mcdata]

Fun fact - we actually just merged in a PR that changes the language of this deprecation warning. This warning was written when metrics were assumed to be in "steady state", which is less true today than it was at the time. The baseline structure of metrics won't change but we'll be making some edits to them in order to make them work well with entities. PR here.. What this means for you is that keeping the old property names around for another minor version won't cause issues - we'll continue to do the behind the scenes conversion to the new names.

Adjusting these properties shouldn't cause any issues for package users as long as they are on 1.3 where these properties are recognized as part of the metric definition.

One thing to note though is that metrics packages are hard-coded to the specific minor version of dbt-core that they're operating with. So our new release candidate in the next few weeks will only work with >=1.4.0, <1.5.0. We're not making any changes to the definition of metrics but if you're materializing metric datasets with the macros, that could cause an issue.

[fivetran-sheringuyen]

I went through and added each metric one by one back into the ad_reporting_metrics.yml and found that the warning was only being triggered for metrics in which a derived expression was used. So in order to fix this issue, for cost_per_click, clickthrough_rate and bounce_rate we need to explicitly declare calculation_method: derived like so:

  - name: clickthrough_rate
    label: Ad Clickthrough Rate (Fivetran)
    description: Percentage of impressions that did convert into clicks.

    calculation_method: derived
    expression: "{{ metric('clicks') }} / {{ metric('impressions') }}"

    timestamp: date_day
    time_grains: [day, week, month]

    dimensions:
      - platform
      - campaign_id
      - campaign_name
      - ad_group_id
      - ad_group_name
      - ad_id
      - ad_name
      - account_id
      - account_name

Will create a PR shortly.

[fivetran-joemarkiewicz]

This issue has been addressed in the latest release and PR highlighted above. As such, I will close out this bug report. Thanks for working through this update @fivetran-sheringuyen!