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-948] [Feature] Adding Metric Tree Parsing Helper Functions to dbt Core #5567

Closed
1 task done
callum-mcdata opened this issue Jul 27, 2022 · 2 comments · Fixed by #5607
Closed
1 task done

[CT-948] [Feature] Adding Metric Tree Parsing Helper Functions to dbt Core #5567

callum-mcdata opened this issue Jul 27, 2022 · 2 comments · Fixed by #5607
Labels
enhancement New feature or request
Milestone

Comments

@callum-mcdata
Copy link
Contributor

Is this your first time opening an issue?

Describe the Feature

Right now, the dbt_metrics package uses a macro called get_metric_tree to traverse the graph in reverse order and get the following information for each metric that is used:

  • full_set: the full list of metrics that are used in the macro call.
  • parent_set: the list of metrics upstream of the metrics used in the macro call. NOT inclusive of expression metrics - this list is only comprised of base metrics
  • expression_set: the list of expression metrics used in the macro call. Inclusive of ones listed or upstrea
  • base_set: the list of metrics listed in the macro call.
  • ordered_expression_set: the list of expression metrics + their depth. Starting at 999 and moving donwards to limit to 1k. Ex:
    • metric a: 999
    • metric b (downstream of a): 998

Helper Functions

This is a list of helper functions that I believe would satisfy the same needs that we currently use the metric tree for:

  1. get_full_metric_list:
    • Function: This function would traverse through the dag in the opposite direction of 1 or more provided metrics and return a list of all metrics upstream of them. Inclusive of metrics listed in input
    • Input: 1+ metrics via the format metric('metric_name')
  2. get_base_metric_list:
    • Function: This function would traverse through the dag in the opposite direction of 1 or more provided metrics and return a list of all base (non-expression) metrics upstream. Inclusive of metrics listed in input
    • Input: 1+ metrics via the format metric('metric_name')
  3. get_expression_metric_list:
    • Function: This function would traverse through the dag in the opposite direction of 1 or more provided metrics and return a list of all expression metrics upstream of them. Inclusive of metrics listed in input
    • Input: 1+ metrics via the format metric('metric_name')
  4. get_ordered_expression_metric_list:
    • Function: This function would traverse through the dag in the opposite direction of 1 or more provided metrics and return a list of all expression metrics upstream of them. It would also contain a numeric representation of the depth of the expression metric Inclusive of metrics listed in input
    • Input: 1+ metrics via the format metric('metric_name')
    • Output: A list of dictionaries like: ['metric_a':1, 'metric_b':2]

I believe we can get away from having a get_base_metric_list because we can have that just be the input from the macro now that we don't need to iterate recursively over a dictionary like we're doing now. Additionally we might be able to get away with just the second expression metric list .`

Describe alternatives you've considered

The alternative is the current implementation in the dbt_metrics package.

Who will this benefit?

Anyone using the dbt_metrics package (ie anyone using the Semantic Layer) and the developer/maintainers of the package!

Are you interested in contributing this feature?

Yes! I'd love to try and write some of these helper functions.

Anything else?

This is the link to the jinja code that currently accomplishes similar functionality.

@callum-mcdata callum-mcdata added enhancement New feature or request triage labels Jul 27, 2022
@github-actions github-actions bot changed the title [Feature] Adding Metric Tree Parsing Helper Functions to dbt Core [CT-948] [Feature] Adding Metric Tree Parsing Helper Functions to dbt Core Jul 27, 2022
@jtcohen6 jtcohen6 added this to the v1.3 milestone Jul 28, 2022
@jtcohen6
Copy link
Contributor

@callum-mcdata Thanks for the thorough write-up! I believe we must prioritize some or all of this refactoring work, ahead of declaring metrics + the dbt-metrics package ready for mature use.

There's one important piece I don't see here, which is constructing the relations for models upstream of metrics. Currently, the dbt-metrics package does that via graph introspection. I think we should investigate using real ref resolution instead, provided by a dbt-core helper. It yielded the issue in #5525. We can resolve that one by other means, but we can't guarantee that we won't run into more weird edge cases.

@callum-mcdata
Copy link
Contributor Author

There's one important piece I don't see here, which is constructing the relations for models upstream of metrics. Currently, the dbt-metrics package does that via graph introspection. I think we should investigate using real ref resolution instead, provided by a dbt-core helper. It yielded the issue in #5525. We can resolve that one by other means, but we can't guarantee that we won't run into more weird edge cases.

I'd hoped using a ref resolution would be a simple swap in the dbt_metrics package but unfortunately it ran into some issues as seen here. Would a dbt-core helper allow us to sidestep the conditional block ref issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants