Add support for percentile metrics (#34)

* Add support for percentile metric types

* Bump version

* Add checks for numerator presence

Author: emielver

eppo_metrics_sync/schema/eppo_metric_schema.json

@@ -118,7 +118,7 @@
             "items": {
                 "type": "object",
                 "additionalProperties": false,
-                "required": ["name", "entity", "numerator"],
+                "required": ["name", "entity"],
                 "properties": {
                     "name": {
                         "description": "A user-friendly name shown in the Eppo UI",
@@ -130,7 +130,7 @@
                     },
                     "type": {
                         "description": "The kind of metric to be calculated",
-                        "enum": ["simple", "ratio", "funnel", "percentile"]
+                        "enum": ["simple", "ratio", "percentile"]
                     },
                     "entity": {
                         "description": "Must exactly match entity's name in Eppo UI",
@@ -315,6 +315,24 @@
                                 "type": "number"
                             }
                         }
+                    },
+                    "percentile": {
+                        "description": "For percentile metrics only: specify the fact and percentile value",
+                        "type": "object",
+                        "additionalProperties": false,
+                        "required": ["fact_name", "percentile_value"],
+                        "properties": {
+                            "fact_name": {
+                                "description": "Must match one of the values specified in fact_sources.facts.name",
+                                "type": "string"
+                            },
+                            "percentile_value": {
+                                "description": "The percentile to calculate (between 0 and 1)",
+                                "type": "number",
+                                "minimum": 0,
+                                "maximum": 1
+                            }
+                        }
                     }
                 }
             }

eppo_metrics_sync/validation.py

@@ -54,9 +54,12 @@ def unique_names(payload):
 def valid_fact_references(payload):
     fact_references = set()
     for metric in payload.metrics:
-        fact_references.add(metric['numerator']['fact_name'])
+        if 'numerator' in metric:
+            fact_references.add(metric['numerator']['fact_name'])
         if 'denominator' in metric:
             fact_references.add(metric['denominator']['fact_name'])
+        if 'percentile' in metric:
+            fact_references.add(metric['percentile']['fact_name'])
 
     fact_names = set()
     for fact_source in payload.fact_sources:
@@ -81,12 +84,22 @@ def valid_experiment_computation(payload):
 
 def metric_aggregation_is_valid(payload):
     for m in payload.metrics:
+        if m.get('type') == 'percentile':
+            percentile_error = percentile_metric_is_valid(m)
+            if percentile_error:
+                payload.validation_errors.append(
+                    f"{m['name']} has invalid percentile configuration: {percentile_error}"
+                )
+            # Skip the rest of the loop iteration for percentile metrics
+            # since they don't have numerator/denominator to validate
+            continue
 
-        numerator_error = aggregation_is_valid(m['numerator'])
-        if numerator_error:
-            payload.validation_errors.append(
-                f"{m['name']} has invalid numerator: {numerator_error}"
-            )
+        if 'numerator' in m:
+            numerator_error = aggregation_is_valid(m['numerator'])
+            if numerator_error:
+                payload.validation_errors.append(
+                    f"{m['name']} has invalid numerator: {numerator_error}"
+                )
 
         if 'denominator' in m:
             denominator_error = aggregation_is_valid(m['denominator'])
@@ -103,13 +116,27 @@ def valid_guardrail_cutoff_signs(payload):
             facts[fact['name']] = fact
 
     for m in payload.metrics:
-        numerator_fact_name = m['numerator']['fact_name']
-        if is_guardrail_cutoff_exist(m) and numerator_fact_name in facts and 'desired_change' in facts[numerator_fact_name]:
-            error = is_valid_guardrail_cutoff_sign(m, facts[numerator_fact_name])
-            if error:
-                payload.validation_errors.append(
-                    f"{m['name']} is having invalid guardrail_cutoff sign: {error}"
-                )
+        if m.get('type') == 'percentile':
+            if is_guardrail_cutoff_exist(m):
+                percentile_fact_name = m['percentile']['fact_name']
+                if percentile_fact_name in facts and 'desired_change' in facts[percentile_fact_name]:
+                    error = is_valid_guardrail_cutoff_sign(m, facts[percentile_fact_name])
+                    if error:
+                        payload.validation_errors.append(
+                            f"{m['name']} is having invalid guardrail_cutoff sign: {error}"
+                        )
+            # Skip the rest of the loop iteration for percentile metrics
+            # since they don't have numerator/denominator to validate
+            continue
+
+        if 'numerator' in m:
+            numerator_fact_name = m['numerator']['fact_name']
+            if is_guardrail_cutoff_exist(m) and numerator_fact_name in facts and 'desired_change' in facts[numerator_fact_name]:
+                error = is_valid_guardrail_cutoff_sign(m, facts[numerator_fact_name])
+                if error:
+                    payload.validation_errors.append(
+                        f"{m['name']} is having invalid guardrail_cutoff sign: {error}"
+                    )
 
 
 def is_valid_guardrail_cutoff_sign(metric, numerator_fact):
@@ -218,3 +245,30 @@ def aggregation_is_valid(aggregation):
         return '\n'.join(error_message)
     else:
         return None
+
+def percentile_metric_is_valid(metric):
+    error_message = []
+
+    # Check for required percentile field
+    if 'percentile' not in metric:
+        error_message.append("Missing 'percentile' field for percentile metric")
+        return '\n'.join(error_message)
+
+    percentile = metric['percentile']
+
+    # Check for required fact_name
+    if 'fact_name' not in percentile:
+        error_message.append("Missing 'fact_name' in percentile configuration")
+
+    # Check for required percentile_value
+    if 'percentile_value' not in percentile:
+        error_message.append("Missing 'percentile_value' in percentile configuration")
+    elif not isinstance(percentile['percentile_value'], (int, float)):
+        error_message.append("'percentile_value' must be a number")
+    elif percentile['percentile_value'] < 0 or percentile['percentile_value'] > 1:
+        error_message.append("'percentile_value' must be between 0 and 1")
+
+    if error_message:
+        return '\n'.join(error_message)
+    else:
+        return None

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "eppo_metrics_sync"
-version = "0.1.7"
+version = "0.1.8"
 description = "Sync metrics to Eppo"
 readme = "README.md"
 requires-python = ">=3.7"

tests/test_validation.py

@@ -156,3 +156,93 @@ def test_valid_yaml():
     eppo_metrics_sync = EppoMetricsSync(directory=None)
     eppo_metrics_sync.load_eppo_yaml(path='tests/yaml/valid/purchases.yaml')
     eppo_metrics_sync.validate()
+
+def test_valid_percentile_yaml():
+    eppo_metrics_sync = EppoMetricsSync(directory=None)
+    eppo_metrics_sync.load_eppo_yaml(path='tests/yaml/valid/percentile_test.yaml')
+    eppo_metrics_sync.validate()
+
+def test_invalid_percentile_yaml():
+    eppo_metrics_sync = EppoMetricsSync(directory=None)
+    eppo_metrics_sync.load_eppo_yaml(path='tests/yaml/invalid/invalid_percentile.yaml')
+
+    with pytest.raises(ValueError) as excinfo:
+        eppo_metrics_sync.validate()
+
+    error_msg = str(excinfo.value)
+    assert "Invalid App opens percentile has invalid percentile configuration: Missing 'percentile_value' in percentile configuration" in error_msg
+    assert "Invalid percentile value has invalid percentile configuration: 'percentile_value' must be between 0 and 1" in error_msg
+
+def test_percentile_metrics():
+    # Test with missing percentile field
+    metric = {
+        'name': 'Test Percentile Metric',
+        'type': 'percentile',
+        'entity': 'User'
+    }
+    from eppo_metrics_sync.validation import percentile_metric_is_valid
+    error = percentile_metric_is_valid(metric)
+    assert error == "Missing 'percentile' field for percentile metric"
+
+    # Test with missing fact_name
+    metric = {
+        'name': 'Test Percentile Metric',
+        'type': 'percentile',
+        'entity': 'User',
+        'percentile': {
+            'percentile_value': 0.95
+        }
+    }
+    error = percentile_metric_is_valid(metric)
+    assert error == "Missing 'fact_name' in percentile configuration"
+
+    # Test with missing percentile_value
+    metric = {
+        'name': 'Test Percentile Metric',
+        'type': 'percentile',
+        'entity': 'User',
+        'percentile': {
+            'fact_name': 'App open'
+        }
+    }
+    error = percentile_metric_is_valid(metric)
+    assert error == "Missing 'percentile_value' in percentile configuration"
+
+    # Test with invalid percentile_value type
+    metric = {
+        'name': 'Test Percentile Metric',
+        'type': 'percentile',
+        'entity': 'User',
+        'percentile': {
+            'fact_name': 'App open',
+            'percentile_value': 'invalid'
+        }
+    }
+    error = percentile_metric_is_valid(metric)
+    assert error == "'percentile_value' must be a number"
+
+    # Test with percentile_value out of range
+    metric = {
+        'name': 'Test Percentile Metric',
+        'type': 'percentile',
+        'entity': 'User',
+        'percentile': {
+            'fact_name': 'App open',
+            'percentile_value': 1.5
+        }
+    }
+    error = percentile_metric_is_valid(metric)
+    assert error == "'percentile_value' must be between 0 and 1"
+
+    # Test with valid percentile metric
+    metric = {
+        'name': 'Test Percentile Metric',
+        'type': 'percentile',
+        'entity': 'User',
+        'percentile': {
+            'fact_name': 'App open',
+            'percentile_value': 0.95
+        }
+    }
+    error = percentile_metric_is_valid(metric)
+    assert error is None

tests/yaml/invalid/invalid_percentile.yaml

@@ -0,0 +1,38 @@
+fact_sources:
+    - name: App Usage
+      sql: |
+          SELECT
+            timestamp as TS,
+            user_id,
+            app_open_duration
+          from customer_db.onboarding.app_usage
+      timestamp_column: TS
+      entities:
+          - entity_name: User
+            column: user_id
+      facts:
+          - name: App open
+            column: app_open_duration
+      reference_url: https://github.com/Eppo-exp/eppo-metrics-sync
+metrics:
+    - name: Invalid App opens percentile
+      description: Missing percentile_value
+      type: percentile
+      entity: User
+      metric_display_style: decimal
+      minimum_detectable_effect: 0.05
+      reference_url: ""
+      percentile:
+          fact_name: App open
+      guardrail_cutoff: null
+    - name: Invalid percentile value
+      description: Percentile value out of range
+      type: percentile
+      entity: User
+      metric_display_style: decimal
+      minimum_detectable_effect: 0.05
+      reference_url: ""
+      percentile:
+          fact_name: App open
+          percentile_value: 2.0
+      guardrail_cutoff: null

tests/yaml/valid/percentile_test.yaml

@@ -0,0 +1,42 @@
+fact_sources:
+    - name: App Usage
+      sql: |
+          SELECT
+            timestamp as TS,
+            user_id,
+            app_open_duration
+          from customer_db.onboarding.app_usage
+      timestamp_column: TS
+      entities:
+          - entity_name: User
+            column: user_id
+          - entity_name: Session
+            column: session_id
+      facts:
+          - name: App open
+            column: app_open_duration
+            desired_change: increase
+      reference_url: https://github.com/Eppo-exp/eppo-metrics-sync
+metrics:
+    - name: App opens (p99)
+      description: User app opens of 99 percentile
+      type: percentile
+      entity: Session
+      metric_display_style: decimal
+      minimum_detectable_effect: 0.05
+      reference_url: ""
+      percentile:
+          fact_name: App open
+          percentile_value: 0.99
+      guardrail_cutoff: null
+    - name: App opens (p90)
+      description: User app opens of 90 percentile
+      type: percentile
+      entity: User
+      metric_display_style: decimal
+      minimum_detectable_effect: 0.05
+      reference_url: ""
+      percentile:
+          fact_name: App open
+          percentile_value: 0.90
+      guardrail_cutoff: null