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

Improve metrics docs & DAG viz #285

Merged
merged 7 commits into from
Jul 5, 2022
Merged

Improve metrics docs & DAG viz #285

merged 7 commits into from
Jul 5, 2022

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Jun 30, 2022

resolves #246

Description

This PR:

  • Draws DAG edges for expression metrics (ie. a metric that depends on one or more other metrics)
  • Renders labels, types (expression vs. aggregate), and SQL for each metric
  • Uses the metric label rather than the name in the sidebar + DAG viz + search results
  • Supports search results on metric + label name (eg. both "arpc" and "Revenue") should pull up the arpc metric

Screenshots

Expression metrics render in the DAG view
Screen Shot 2022-06-30 at 4 27 54 PM

Render the name, label, and type for each metric
Screen Shot 2022-06-30 at 4 28 25 PM

Example metric SQL definition:

select sum(order_total)
from {{ ref('fct_orders') }}
where is_paid = true

Checklist

  • I have signed the CLA
  • I have generated docs locally, and this change appears to resolve the stated issue
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Jun 30, 2022
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I confirmed with some testing that it's behaving as expected. However, I'd love to get @stu-k's eyes on this before merging it in.

@emmyoop emmyoop requested a review from stu-k July 5, 2022 12:29
Copy link
Contributor

@stu-k stu-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nitpicky comments about consistency and code style, all of this seems functional as it is now 👍

src/app/components/search/search.js Outdated Show resolved Hide resolved
src/app/docs/metric.js Outdated Show resolved Hide resolved
src/app/services/code_service.js Outdated Show resolved Hide resolved
drewbanin and others added 3 commits July 5, 2022 14:34
Co-authored-by: Stu Kilgore <stuart.kilgore@gmail.com>
Co-authored-by: Stu Kilgore <stuart.kilgore@gmail.com>
Co-authored-by: Stu Kilgore <stuart.kilgore@gmail.com>
@drewbanin drewbanin marked this pull request as ready for review July 5, 2022 18:37
@drewbanin
Copy link
Contributor Author

@stu-k! Thank you so much for this review! I just moved this out of the draft state - lmk if there's anything else I should do before merging! :D

@stu-k
Copy link
Contributor

stu-k commented Jul 5, 2022

@drewbanin As long as you tested locally and everything is working as expected, it should be good to go!

@drewbanin
Copy link
Contributor Author

right on - I pulled these new commits down and tested metrics on local. Merging!

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

Successfully merging this pull request may close these issues.

Additions to metrics documentations
3 participants