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

Making Metric Timestamps Optional #6402

Merged
merged 8 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changes/unreleased/Features-20221207-091722.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Features
body: Making timestamp optional for metrics
time: 2022-12-07T09:17:22.571877-06:00
custom:
Author: callum-mcdata
Issue: "6398"
PR: "9400"
2 changes: 1 addition & 1 deletion core/dbt/contracts/graph/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -976,12 +976,12 @@ class Metric(GraphNode):
description: str
label: str
calculation_method: str
timestamp: str
expression: str
filters: List[MetricFilter]
time_grains: List[str]
dimensions: List[str]
resource_type: NodeType = field(metadata={"restrict": [NodeType.Metric]})
timestamp: Optional[str] = None
window: Optional[MetricTime] = None
model: Optional[str] = None
model_unique_id: Optional[str] = None
Expand Down
12 changes: 11 additions & 1 deletion core/dbt/contracts/graph/unparsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -484,9 +484,9 @@ class UnparsedMetric(dbtClassMixin, Replaceable):
name: str
label: str
calculation_method: str
timestamp: str
expression: str
description: str = ""
timestamp: Optional[str] = None
time_grains: List[str] = field(default_factory=list)
dimensions: List[str] = field(default_factory=list)
window: Optional[MetricTime] = None
Expand Down Expand Up @@ -518,6 +518,16 @@ def validate(cls, data):
f"The metric name '{data['name']}' is invalid. It {', '.join(e for e in errors)}"
)

if data.get("timestamp") is None and data.get("time_grains") is not None:
raise ValidationError(
f"The metric '{data['name']} has time_grains defined but is missing a timestamp dimension."
)

if data.get("timestamp") is None and data.get("window") is not None:
raise ValidationError(
f"The metric '{data['name']} has a window defined but is missing a timestamp dimension."
)

if data.get("model") is None and data.get("calculation_method") != "derived":
raise ValidationError("Non-derived metrics require a 'model' property")

Expand Down
55 changes: 55 additions & 0 deletions tests/functional/metrics/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -642,3 +642,58 @@
meta:
my_meta: 'testing'
"""

metric_without_timestamp_or_timegrains_yml = """
version: 2

metrics:
- name: number_of_people
label: "Number of people"
description: Total count of people
model: "ref('people')"
calculation_method: count
expression: "*"
dimensions:
- favorite_color
- loves_dbt
meta:
my_meta: 'testing'
"""

invalid_metric_without_timestamp_with_time_grains_yml = """
version: 2

metrics:
- name: number_of_people
label: "Number of people"
description: Total count of people
model: "ref('people')"
time_grains: [day, week, month]
calculation_method: count
expression: "*"
dimensions:
- favorite_color
- loves_dbt
meta:
my_meta: 'testing'
"""

invalid_metric_without_timestamp_with_window_yml = """
version: 2

metrics:
- name: number_of_people
label: "Number of people"
description: Total count of people
model: "ref('people')"
window:
count: 14
period: day
calculation_method: count
expression: "*"
dimensions:
- favorite_color
- loves_dbt
meta:
my_meta: 'testing'
"""
69 changes: 69 additions & 0 deletions tests/functional/metrics/test_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from dbt.tests.util import run_dbt, get_manifest
from dbt.exceptions import ParsingException


from tests.functional.metrics.fixtures import (
mock_purchase_data_csv,
models_people_sql,
Expand All @@ -18,6 +19,9 @@
invalid_derived_metric_contains_model_yml,
derived_metric_yml,
derived_metric_old_attr_names_yml,
metric_without_timestamp_or_timegrains_yml,
invalid_metric_without_timestamp_with_time_grains_yml,
invalid_metric_without_timestamp_with_window_yml
)


Expand Down Expand Up @@ -46,6 +50,33 @@ def test_simple_metric(
assert metric_ids == expected_metric_ids


class TestSimpleMetricsNoTimestamp:
@pytest.fixture(scope="class")
def models(self):
return {
"people_metrics.yml": metric_without_timestamp_or_timegrains_yml,
"people.sql": models_people_sql,
}

def test_simple_metric_no_timestamp(
self,
project,
):
# initial run
results = run_dbt(["run"])
assert len(results) == 1
manifest = get_manifest(project.project_root)
metric_ids = list(manifest.metrics.keys())
expected_metric_ids = [
"metric.test.number_of_people",
]
assert metric_ids == expected_metric_ids

# make sure the 'expression' metric depends on the two upstream metrics
metric_test = manifest.metrics["metric.test.number_of_people"]
assert metric_test.timestamp is None


class TestInvalidRefMetrics:
@pytest.fixture(scope="class")
def models(self):
Expand Down Expand Up @@ -253,3 +284,41 @@ def models(self):
"derived_metric.yml": derived_metric_old_attr_names_yml,
"downstream_model.sql": downstream_model_sql,
}


class TestInvalidTimestampTimeGrainsMetrics:
@pytest.fixture(scope="class")
def models(self):
return {
"people_metrics.yml": invalid_metric_without_timestamp_with_time_grains_yml,
"people.sql": models_people_sql,
}

# Tests that we get a ParsingException with an invalid metric definition.
# This metric definition is missing timestamp but HAS a time_grains property
def test_simple_metric(
self,
project,
):
# initial run
with pytest.raises(ParsingException):
run_dbt(["run"])


class TestInvalidTimestampWindowMetrics:
@pytest.fixture(scope="class")
def models(self):
return {
"people_metrics.yml": invalid_metric_without_timestamp_with_window_yml,
"people.sql": models_people_sql,
}

# Tests that we get a ParsingException with an invalid metric definition.
# This metric definition is missing timestamp but HAS a window property
def test_simple_metric(
self,
project,
):
# initial run
with pytest.raises(ParsingException):
run_dbt(["run"])