feat(analytics): metric definitions listing endpoint - #1860
Conversation
Expose the unified metric definitions as a read-only display listing: every definition visible to the tenant (tenant override wins per key), sorted by metric_key, with is_enabled and schema_status reported rather than filtered. Computation internals stay off the wire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 0bf278e5f91178a4915067b7ad943925b7ec723b and e898fec. 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds an authenticated ChangesMetric catalog API
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)docs/components/backend/analytics/openapi.jsonTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/services/analytics/src/domain/metric_definitions/repository.rs (1)
619-653: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
COALESCEdoesn't fully enforce the stated monotonic-freshness invariant.The comment says
last_observed_date is monotonic knowledge, not per-sweep state, butCOALESCE(?, last_observed_date)only protects the "no observation produced" (NULL) case. If a sweep computes alast_observedthat is older than the currently stored value (e.g., a ClickHouse TTL/partition drop or backfill temporarily lowersmax(metric_date)for a measure), this silently regresses the freshness marker instead of preserving the higher-water mark.🔧 Suggested direction: never let the marker move backward
- last_observed_date = COALESCE(?, last_observed_date), \ + last_observed_date = CASE \ + WHEN ? IS NULL THEN last_observed_date \ + WHEN last_observed_date IS NULL OR ? > last_observed_date THEN ? \ + ELSE last_observed_date \ + END, \ updated_at = updated_at \ WHERE id = ?", [ Value::from(status.as_db()), match error_code { Some(code) => Value::from(code.as_db()), None => Value::String(None), }, - match last_observed { - Some(date) => Value::from(date.to_string()), - None => Value::String(None), - }, + // bound three times, matching the three `?` placeholders in the CASE above + last_observed_value(last_observed), + last_observed_value(last_observed), + last_observed_value(last_observed), uuid_value(definition_id), ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/analytics/src/domain/metric_definitions/repository.rs` around lines 619 - 653, Update update_definition_status so last_observed_date remains monotonic: preserve the existing database value when the supplied last_observed date is NULL or older than the stored date, and only advance it when the supplied date is newer. Implement this in the SQL update expression while keeping the existing status, error code, and timestamp behavior unchanged.
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/api/test_metric_definitions.py (1)
37-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test asserts tenant-override values actually win, only that keys collapse to one row.
test_list_metric_definitions_sorted_and_uniqueproves uniqueness/sorting but not that an override's fields (e.g.label) are the ones returned rather than the product-default's. Given this precedence rule is central to the endpoint's contract, consider seeding a tenant override for a knownmetric_keyand asserting the returned row reflects the override, not the default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/api/test_metric_definitions.py` around lines 37 - 45, Extend test_list_metric_definitions_sorted_and_unique to seed a tenant override for a known metric_key with a distinguishable field value, then locate that returned metric and assert its overridden field, such as label, is returned instead of the product-default value. Preserve the existing sorted-key and uniqueness assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/services/analytics/src/domain/metric_definitions/validator.rs`:
- Around line 223-257: Update check_dimension_coverage and its
dimension_coverage query to anchor each measure’s coverage window using that
measure’s own entry in last_dates, rather than the definition-wide last_observed
maximum. Preserve batching where practical, but ensure stale measures are
evaluated against their own observation date so freshness differences cannot
produce a false DimensionNotCovered result; retain the existing behavior for
measures without observed dates.
---
Outside diff comments:
In `@src/backend/services/analytics/src/domain/metric_definitions/repository.rs`:
- Around line 619-653: Update update_definition_status so last_observed_date
remains monotonic: preserve the existing database value when the supplied
last_observed date is NULL or older than the stored date, and only advance it
when the supplied date is newer. Implement this in the SQL update expression
while keeping the existing status, error code, and timestamp behavior unchanged.
---
Nitpick comments:
In `@src/ingestion/tests/e2e/api/test_metric_definitions.py`:
- Around line 37-45: Extend test_list_metric_definitions_sorted_and_unique to
seed a tenant override for a known metric_key with a distinguishable field
value, then locate that returned metric and assert its overridden field, such as
label, is returned instead of the product-default value. Preserve the existing
sorted-key and uniqueness assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b200c7d-d1ae-48d3-b132-e998f2c084ba
📥 Commits
Reviewing files that changed from the base of the PR and between fd92d5b and 0bf278e5f91178a4915067b7ad943925b7ec723b.
📒 Files selected for processing (12)
docs/components/backend/analytics/openapi.jsonsrc/backend/services/analytics/src/api/metric_definitions.rssrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/domain/metric_definitions/definition.rssrc/backend/services/analytics/src/domain/metric_definitions/error_code.rssrc/backend/services/analytics/src/domain/metric_definitions/listing.rssrc/backend/services/analytics/src/domain/metric_definitions/mod.rssrc/backend/services/analytics/src/domain/metric_definitions/repository.rssrc/backend/services/analytics/src/domain/metric_definitions/validator.rssrc/backend/services/analytics/src/migration/m20260722_000001_metric_definition_last_observed.rssrc/backend/services/analytics/src/migration/mod.rssrc/ingestion/tests/e2e/api/test_metric_definitions.py
Add last_observed_date to metric definitions: the schema validator now records max(metric_date) per definition and probes measure existence over all history, so schema_status is purely structural and freshness is tracked separately. Dimension coverage is anchored at the newest observed row rather than today(). Also surface schema_error_code on the listing so an error status carries its cause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
0bf278e to
1c9ce42
Compare
…st coverage - Dimension coverage windows each measure at its own newest observation rather than the definition-wide max, so a stale measure can't yield a false DimensionNotCovered. - update_definition_status advances last_observed_date only on a strictly newer date; a NULL or older sweep result never regresses it. - Separate the pure decision, SQL-building, and row-mapping logic from the ClickHouse and MariaDB I/O wrappers and unit-test it; add MariaDB live tests for the listing read path and the status writer, and an e2e tenant-override assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
…ns-endpoint Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar> # Conflicts: # docs/components/backend/analytics/openapi.json
Adds read-only
GET /v1/metric-definitions— lists every metric definition visible to the tenant (tenant override wins per key), sorted bymetric_key, display fields only (no computation internals on the wire).Also decouples schema validity from data freshness:
schema_statusis purely structural (a quiet source no longer reads as broken).last_observed_daterecordsmax(metric_date)per definition as a separate freshness signal.schema_error_codesurfaced so an error status carries its cause.today().OpenAPI regenerated; unit + e2e contract tests added.
Closes #1859
Summary by CodeRabbit
New Features
Documentation