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

Metrics Parsing #39

Closed
wants to merge 143 commits into from
Closed

Metrics Parsing #39

wants to merge 143 commits into from

Conversation

z3z1ma
Copy link
Contributor

@z3z1ma z3z1ma commented Jul 20, 2021

Description

Bring your metrics into revision control, bring them into your data model, sync it directly with Metabase.

dbt-metabase metrics \
  --dbt_database test \
  --dbt_path tests/fixtures/metric/ \
   --schema public \
   --metabase_host localhost:3000 \
   --metabase_user alex@... \
   --metabase_password "..." \
   --metabase_database unit_testing \
   --metabase_use_http --verbose

Expression syntax: https://www.metabase.com/docs/latest/users-guide/expressions.html

  - name: Number of Customers with Large Orders
    description: Customers who are big spenders should be tracked independently of total,
      any customer who orders over 20 AUD of jaffle is counted
    metric: countif([customer_lifetime_value] > 20)

We will parse: countif([customer_lifetime_value] > 20) into['count-where', ['>', ['field', 41, None], 20]]
The parser should be able to handle any type of expression allowing users to use the nice excel like syntax built by metabase team directly alongside your data models. They become centralized, self contained, and gain all the advantages of dbt/jinja.

image

Parsing Examples

Purposefully convoluted examples showing robustness and possibilities (most metrics are simple in theory with preprocessing logic in the model)

Input: Sum(case([site_dispenser_count] + 1 > 1 or [site_dispenser_count] - 1 > 1, [site_dispenser_count] + 1))
Output: ['sum', ['case', [[['or', ['>', ['+', ['field', 1, 'site_dispenser_count'], 1], 1], ['>', ['-', ['field', 1, 'site_dispenser_count'], 1], 1]], ['+', ['field', 1, 'site_dispenser_count'], 1]]]]]

Input: Sum([table.order] + [qty] * 2 + 4 + 5 - 4 + 5)
Output: ['sum', ['+', ['-', ['+', ['field', 1, 'table.order'], ['*', ['field', 1, 'qty'], 2], 4, 5], 4], 5]]

Input: SumIf([site_dispenser_count], [site_city] > "Phoenix" or [site_state] = "Arizona")
Output: ['sum-where', ['field', 1, 'site_dispenser_count'], ['or', ['>', ['field', 1, 'site_city'], '"Phoenix"'], ['=', ['field', 1, 'site_state'], '"Arizona"']]]

Input: Distinct(case([site_city] = "Phoenix", [site_panel_count])) / distinct([site_dispenser_count])
Output: ['/', ['distinct', ['case', [[['=', ['field', 1, 'site_city'], '"Phoenix"'], ['field', 1, 'site_panel_count']]]]], ['distinct', ['field', 1, 'site_dispenser_count']]]

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • CI test for metric propagation and synchronization

Test Configuration:

  • Python version: 3.8

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

References #25

falador_wiz1 and others added 30 commits June 12, 2021 14:06
…ersion of dbt-metabase having improved yaml parsing. large formatting update to conform to black.
…special or semantic until ready to deprecate
…e properly used if found in metabase api response.
… without depends on/test_metadata dont throw
…xpected input format to be defined in readme. otherwise automatic resolution of target field using relation test will prepend target run schema which should be fine in 95% of use cases. cases outside that can use manifest.json parsing or set fk_ref in yml.
…d ensured support for schema agnostic fk targets (schema resolved from manifest.json)
…sync will now only fail hard if timeout is explicit, otherwise default behaviour if --sync is true is to attempt sync for 30 seconds and proceed with aligning what can be aligned successfully. more formatting and a few comments for clarity of intent. also added option to pass custom cert bundle to verify.
…usly with seemless function alongside primary artifact parser (manifest.json).
…f regex to permit catching last arg of either ref or source always being the target table. if pointing to an alias, we are collecting aliases during yml parsing to be passed to metabased client and translated to metabase table names as needed. this functionality should be unnoticed by the user but provide more resiliency as well as more user friendly outcome whilst still being very specific in our logging.
…on path strings allowing relative paths for --dbt_path or --dbt_manifest_path simplified
…ntees us `schema.table` format. This allows us to guarantee the incorrectly formatted ref (which should be `schema.table`) is originating from yml. Log the warning and infer correct schema for our users using target schema which covers the 90% use case.
…nit__ to nonemptystr class, cleaned some logging calls to use lazy interpolation
@gouline gouline added this to the v0.9.x milestone Jul 28, 2021
@remigabillet
Copy link
Contributor

hey @z3z1ma What's the latest on this PR btw? I think this feature is HUGE! I'd love to see it land.

@z3z1ma
Copy link
Contributor Author

z3z1ma commented Sep 20, 2021

hey @z3z1ma What's the latest on this PR btw? I think this feature is HUGE! I'd love to see it land.

Hey @remigabillet

Yeah totally. I think I just need to rebase on the latest stable master branch and put in a couple unit tests. It's actually not much left to do at all provided there aren't too many conflicts. The metric parser is in its own module so I expect it to be easy. Let me run this down so we get a branch in the main repo we can use until goulline is ready to merge it to an RC. I'll handle this, this week. It is indeed a superpowered feature.

@remigabillet
Copy link
Contributor

Sounds great @z3z1ma. Ping me when it's ready for review.

@z3z1ma
Copy link
Contributor Author

z3z1ma commented Oct 3, 2021

closing this in favor of #66 which is the same PR but from a fresh branch

@z3z1ma z3z1ma closed this Oct 3, 2021
@z3z1ma z3z1ma mentioned this pull request Oct 3, 2021
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants