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

feat: Metric Parsing #66

Closed
wants to merge 23 commits into from
Closed

Conversation

z3z1ma
Copy link
Contributor

@z3z1ma z3z1ma commented Oct 3, 2021

NOTE: I am moving the original PR #39 to here since we have had enough changes since then that it was easier to integrate freshly. This branch is fully functional for any eager users/early testers and is on the roadmap for the 0.9.0 release.

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 23 commits September 25, 2021 22:09
…tract interface complexity, allow env vars implicitly
…rt, iteractive config generation and updates
…ookup which lets us reimplement required args
@z3z1ma z3z1ma mentioned this pull request Oct 3, 2021
10 tasks
@z3z1ma z3z1ma changed the title Feat/metric parsing feat: Metric Parsing Oct 3, 2021
Copy link
Contributor

@remigabillet remigabillet left a comment

Choose a reason for hiding this comment

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

I'm very excited about this work. Being able to control Metabase metrics directly from dbt is huge.
Anyway, I've been using this branch and found a bug in the compiler. I'll keep trying it and will keep you posted if I find more issues.

},
# boolean operators
"and": {"displayName": "AND", "type": "boolean", "args": ["boolean", "boolean"]},
"or": {"displayName": "OR", "type": "boolean", "args": ["boolean", "boolean"]},
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed a bug in the compiler with the following expression: metabase_compiler.transpile_expression("CountIf(isnull([startup_pitched_at] OR NOT isnull([startup_pitched_at])))")
which compiles to ['count']

It seems to be related to "NOT OR" because if I remove the NOT operator it works as expected
metabase_compiler.transpile_expression("CountIf(isnull([startup_pitched_at] OR isnull([startup_pitched_at])))")
=> ['count-where', ['is-null', ['or', ['field', 1417549, None], ['is-null', ['field', 1417549, None]]]]]

or if I use parenthesis, metabase_compiler.transpile_expression("CountIf(isnull([startup_pitched_at] OR (NOT isnull([startup_pitched_at]))))")
=> ['count-where', ['is-null', ['or', ['field', 1417549, None], ['not', ['is-null', ['field', 1417549, None]]]]]]

The expression at the top is valid in Metabase and was copy/pasted from the UI so I would hope it works in this compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this @remigabillet ! I will take a look at the compiler during the break. Think we are ready to move this into a beta in the master branch with a little documentation, variable renaming, etc. Need to look more deeply into dbt's metrics definitions too are they push along their 1.0 rc's.

@remigabillet
Copy link
Contributor

@z3z1ma what's the latest on getting this work merged?

@gouline gouline added the stalled label May 1, 2022
@gouline gouline closed this Jul 6, 2022
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.

3 participants