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-1130] [Feature] Update Metric Spec With Clearer Names #5774

Closed
3 tasks done
callum-mcdata opened this issue Sep 6, 2022 · 1 comment · Fixed by #5775
Closed
3 tasks done

[CT-1130] [Feature] Update Metric Spec With Clearer Names #5774

callum-mcdata opened this issue Sep 6, 2022 · 1 comment · Fixed by #5775
Assignees
Labels
enhancement New feature or request help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors semantic Issues related to the semantic layer
Milestone

Comments

@callum-mcdata
Copy link
Contributor

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

Technically a duplicate of this issue #4325

Right now, we have two fields within the metric spec whose names don't perfectly match what they are trying to convey.

  • sql : this attribute represents the sql expression that will be aggregated. It can be a simple field (user_id) or more complicated (case when x > y then x else y end)
  • type : this attribute represents the calculation / aggregation method that we are performing on the expression.

Proposed Change

As such, I propose the following changes:

  • Renaming type to calculation_method.
    • Why calculation method and not aggregation method? This is because we do have some metrics that aren't aggregations. Specifically expression/derived metrics. Calculation method keeps the door open to other metric types in the future that also aren't technically aggregations
  • Renaming sql to expression
    • Alternatively we could call this measure - this would build off the prior art of a lot of BI tools.
  • Renaming expression type metrics to derived metrics
    • This was the top suggestion for name by the community. I have no strong opinions on this one so I will bow to the will of the community
Old format
version: 2 
metrics:
  - name: base_average_metric
    type: average
    sql: discount_total

    model: ref('fact_orders')
    label: Total Discount ($)
    timestamp: order_date
    time_grains: [day, week, month, all_time]
    dimensions:
      - had_discount
      - order_country
New format
version: 2 
metrics:
  - name: base_average_metric
    calculation_method: average
    expression: discount_total

    model: ref('fact_orders')
    label: Total Discount ($)
    timestamp: order_date
    time_grains: [day, week, month, all_time]
    dimensions:
      - had_discount
      - order_country

Let me know what y'all think!

Describe alternatives you've considered

Not changing this.

Who will this benefit?

Anyone who uses metrics!

Are you interested in contributing this feature?

Yep! Already scoping it out in a PR

Anything else?

No response

@callum-mcdata callum-mcdata added enhancement New feature or request triage labels Sep 6, 2022
@github-actions github-actions bot changed the title [Feature] Update Metric Spec With Clearer Names [CT-1130] [Feature] Update Metric Spec With Clearer Names Sep 6, 2022
@jtcohen6 jtcohen6 added help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors and removed triage labels Sep 7, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Sep 7, 2022

@callum-mcdata Thanks for taking charge of the Slack convo + issue + PR!

@jtcohen6 jtcohen6 added this to the v1.3 milestone Sep 7, 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
enhancement New feature or request help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors semantic Issues related to the semantic layer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants