feat(metrics): add evidence metadata and drilldown capability - #2072
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 436ce21f054882022eae2591bbecd5163be36f99 and 0862081. 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughAdds evidence metadata persistence and validation, derives fail-closed metric drilldown capabilities, and exposes those capabilities plus canonical selection context through metric definitions and metric results APIs. ChangesEvidence Drilldown Capability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant query_metric_results
participant load_capabilities
Client->>query_metric_results: request metric results
query_metric_results->>load_capabilities: load tenant metric capabilities
load_capabilities-->>query_metric_results: return capability map
query_metric_results-->>Client: return selection and optional drilldown metadata
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
aleksdotbar
added
the
stack:metric-drilldown
509cc93 to
2bb09fb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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_drilldown/mod.rs (1)
108-117: 📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
cargo fmt --checkfailure (confirmed by pipeline logs).Two consecutive blank lines before
fn db_error— rustfmt collapses these to one, which is exactly what the CI fmt-check diff reports.🔧 Proposed fix
Ok(capabilities) } - fn db_error(error: &sea_orm::DbErr) -> CanonicalError { tracing::error!(error = %error, "metric drilldown metadata query failed"); CanonicalError::internal("failed to load metric evidence metadata").create() }🤖 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_drilldown/mod.rs` around lines 108 - 117, Remove the extra blank line immediately before the db_error function so only one blank line separates it from the preceding code, allowing cargo fmt --check to pass.Source: Pipeline failures
🧹 Nitpick comments (2)
src/backend/services/analytics/src/domain/metric_definitions/repository.rs (1)
638-665: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMirror the stale-revision logging added to
update_evidence_status.
update_source_statusnow silently returnsOk(())when the revision guard matches nothing, andvalidate_allthen proceeds tovalidate_definitions_for_sourceas if the write landed. Emitting the samerows_affected == 0trace keeps the two writers diagnosable.🔍 Proposed change
- db.execute(Statement::from_sql_and_values( + let result = db + .execute(Statement::from_sql_and_values( db.get_database_backend(),)) .await?; + if result.rows_affected() == 0 { + tracing::trace!( + %source_id, + config_revision, + "metric source status update skipped for stale configuration revision" + ); + } Ok(()) }🤖 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 638 - 665, Update update_source_status to capture the execute result, inspect rows_affected, and emit the same stale-revision trace used by update_evidence_status when no row matches; preserve the existing SQL, parameters, and return behavior.src/backend/services/analytics/src/domain/metric_results/builder.rs (1)
286-298: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEmpty-string placeholder selection depends on the caller overwriting it.
build_metric_resultemitsselectionwithentity.type = ""andperiod.from/to = "", and correctness relies entirely onquery_metric_resultsreplacing the whole struct afterwards. Any future caller silently ships a response whose canonical selection claims an empty entity type and non-ISO empty dates.Passing the selection (or the validated request + filters) into
build_metric_resultmakes the contract non-optional; there is only one production call site today.♻️ Sketch
pub fn build_metric_result( def: &MetricDefinition, views: Vec<MetricResultViewDto>, + selection: MetricResultSelectionDto, ) -> MetricResultDto {drilldown: None, - selection: super::dto::MetricResultSelectionDto { - metric_key: def.key().to_owned(), - entity: super::dto::MetricResultsEntityDto { - r#type: String::new(), - ids: Vec::new(), - }, - period: super::dto::MetricResultsPeriodDto { - from: String::new(), - to: String::new(), - }, - filters: Vec::new(), - }, + selection,🤖 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_results/builder.rs` around lines 286 - 298, Update build_metric_result to accept the validated selection or request and filters as an argument, then populate MetricResultSelectionDto from those values instead of empty-string placeholders. Update query_metric_results, the sole production caller, to pass the validated data through so every result carries the canonical entity type and ISO period values without relying on later replacement.
🤖 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 464-562: The observation-since query in
evidence_granularities_match currently scans all historical metric_date values
before applying the evidence window, allowing read limits to fail. Bound this
freshness probe to a recent metric_date range before computing max(metric_date),
reusing the existing probe-window configuration and preserving the subsequent
evidence-window matching behavior.
In
`@src/backend/services/analytics/src/migration/m20260727_000001_metric_evidence.rs`:
- Around line 58-78: The replace_evidence_constraints function currently drops
existing checks with DROP CHECK; update its generated drop DDL to use DROP
CONSTRAINT IF EXISTS for each constraint name, preserving the subsequent
constraint creation flow.
---
Outside diff comments:
In `@src/backend/services/analytics/src/domain/metric_drilldown/mod.rs`:
- Around line 108-117: Remove the extra blank line immediately before the
db_error function so only one blank line separates it from the preceding code,
allowing cargo fmt --check to pass.
---
Nitpick comments:
In `@src/backend/services/analytics/src/domain/metric_definitions/repository.rs`:
- Around line 638-665: Update update_source_status to capture the execute
result, inspect rows_affected, and emit the same stale-revision trace used by
update_evidence_status when no row matches; preserve the existing SQL,
parameters, and return behavior.
In `@src/backend/services/analytics/src/domain/metric_results/builder.rs`:
- Around line 286-298: Update build_metric_result to accept the validated
selection or request and filters as an argument, then populate
MetricResultSelectionDto from those values instead of empty-string placeholders.
Update query_metric_results, the sole production caller, to pass the validated
data through so every result carries the canonical entity type and ISO period
values without relying on later replacement.
🪄 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 Plus
Run ID: 743f0f68-9ba2-42c2-9119-38b59ef44f78
📥 Commits
Reviewing files that changed from the base of the PR and between 99697d1 and 2bb09fbafc759324da91905737691c7567815eb8.
📒 Files selected for processing (20)
docs/components/backend/analytics/openapi.jsondocs/domain/metrics/specs/DESIGN.mdsrc/backend/services/analytics/src/api/metric_results.rssrc/backend/services/analytics/src/domain/metric_definitions/builtin.rssrc/backend/services/analytics/src/domain/metric_definitions/definition.rssrc/backend/services/analytics/src/domain/metric_definitions/listing.rssrc/backend/services/analytics/src/domain/metric_definitions/live_tests.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/seeds.rssrc/backend/services/analytics/src/domain/metric_definitions/test_fixture.rssrc/backend/services/analytics/src/domain/metric_definitions/validator.rssrc/backend/services/analytics/src/domain/metric_drilldown/mod.rssrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/dto.rssrc/backend/services/analytics/src/domain/metric_results/mod.rssrc/backend/services/analytics/src/domain/metric_results/validation.rssrc/backend/services/analytics/src/domain/mod.rssrc/backend/services/analytics/src/migration/m20260727_000001_metric_evidence.rssrc/backend/services/analytics/src/migration/mod.rs
2bb09fb to
be02253
Compare
be02253 to
436ce21
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/components/backend/analytics/openapi.json (1)
1237-1251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare canonical period bounds as dates.
The handler serializes validated date bounds, but this schema advertises arbitrary strings. Add
format: "date"for both fields (at the generation source if this file is generated).Proposed schema update
"from": { + "format": "date", "type": "string" }, "to": { + "format": "date", "type": "string" }🤖 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 `@docs/components/backend/analytics/openapi.json` around lines 1237 - 1251, Update the MetricResultsPeriodDto schema definition so the from and to properties declare the date format while retaining their string types and required status. If this OpenAPI document is generated, apply the change at its schema-generation source rather than editing only the generated output.src/backend/services/analytics/src/domain/metric_results/builder.rs (1)
766-808: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new response fields in the wire-shape test.
The test passes
selection(...)but never verifies it serialized; also assert that theNonedrilldown is omitted.Proposed test coverage
assert_eq!(sum_json["metric_key"], "ai.accepted_lines"); assert_eq!(sum_json["format"], "integer"); assert!(sum_json.get("scale").is_none()); + assert_eq!(sum_json["selection"]["metric_key"], "ai.accepted_lines"); + assert_eq!(sum_json["selection"]["entity"]["type"], "person"); + assert_eq!(sum_json["selection"]["period"]["from"], "2026-07-01"); + assert!(sum_json.get("drilldown").is_none());🤖 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_results/builder.rs` around lines 766 - 808, Update metric_result_wire_shape_is_flat_with_computation_tag to assert the serialized selection fields from selection(...) are present, including metric_key, entity, period, and filters as appropriate, and verify that the None drilldown field is omitted from the JSON. Keep the existing computation, format, and scale assertions unchanged.src/backend/services/analytics/src/domain/metric_definitions/validator.rs (1)
130-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDistinguish "no evidence configured" from "evidence_ref is invalid".
Both collapse into
ValidationState::Unchecked. Drilldown still fails closed downstream, but an operator who typosevidence_ref(e.g.ai_evidence) gets the same status as a source that never opted into evidence, with no error code and no warning.♻️ Proposed change
let state = match ( evidence_ref.and_then(EvidenceRelation::parse), ObservationRelation::parse(source_ref), ) { @@ - _ => Some(ValidationState::Unchecked), + _ if evidence_ref.is_some_and(|value| EvidenceRelation::parse(value).is_none()) => { + tracing::warn!(source_key, evidence_ref, "unparseable evidence relation"); + Some(ValidationState::Error(MetricSchemaErrorCode::Unknown)) + } + _ => Some(ValidationState::Unchecked), };🤖 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/validator.rs` around lines 130 - 178, Update the state match around EvidenceRelation::parse so a missing evidence_ref remains ValidationState::Unchecked, while a present but invalid evidence_ref returns an appropriate ValidationState::Error with the existing schema error code and emits a warning identifying the invalid reference. Preserve the existing validation flow for valid evidence references and observation relations.src/backend/services/analytics/src/domain/metric_drilldown/mod.rs (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClickHouse query budgets live in the wrong module.
These four constants are consumed only by
metric_definitions/validator.rs;metric_drilldownitself never issues a ClickHouse query. Consider hosting them with the validator (or a shared probe-budget module) so the drilldown module stays a pure DB/capability concern.🤖 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_drilldown/mod.rs` around lines 11 - 14, Move EVIDENCE_QUERY_TIMEOUT_SECS, EVIDENCE_QUERY_MEMORY_BYTES, EVIDENCE_QUERY_READ_BYTES, and EVIDENCE_QUERY_RESULT_BYTES out of metric_drilldown and colocate them with the consumer in metric_definitions/validator.rs, or a shared probe-budget module if appropriate. Update the validator imports/references to use their new location and leave metric_drilldown focused on database and capability concerns.src/backend/services/analytics/src/domain/metric_definitions/repository.rs (1)
635-662: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
uuid_valuehelper and move the pinning comment above both writers.
update_source_statushand-rolls the UUIDValue::Bytesthatuuid_value(Line 750) already provides, andupdate_evidence_statususes the helper. The comment reads "the status writers below" but the evidence writer sits above it, so both writers are only partially covered.♻️ Proposed cleanup
-// `updated_at = updated_at` in the status writers below pins the column so -// ON UPDATE CURRENT_TIMESTAMP(3) does not fire: updated_at tracks config -// edits, not validator sweeps. pub async fn update_source_status( @@ - Value::Bytes(Some(Box::new(source_id.as_bytes().to_vec()))), + uuid_value(source_id), Value::from(config_revision),and place the pinning comment immediately above
update_evidence_statusso it documents both writers.🤖 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 635 - 662, Update update_source_status to reuse the existing uuid_value helper for source_id instead of constructing a Value::Bytes manually. Move the updated_at pinning comment from above update_source_status to immediately above update_evidence_status so it documents both status writers.
🤖 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/migration/m20260727_000001_metric_evidence.rs`:
- Around line 84-93: Update the trg_metric_sources_evidence_ref_invalidate
trigger so it also detects changes to source_ref, alongside evidence_ref, and
resets evidence_schema_status, evidence_schema_checked_at, and
evidence_schema_error_code whenever either relation changes. Preserve the
existing invalidation behavior and trigger scope.
---
Nitpick comments:
In `@docs/components/backend/analytics/openapi.json`:
- Around line 1237-1251: Update the MetricResultsPeriodDto schema definition so
the from and to properties declare the date format while retaining their string
types and required status. If this OpenAPI document is generated, apply the
change at its schema-generation source rather than editing only the generated
output.
In `@src/backend/services/analytics/src/domain/metric_definitions/repository.rs`:
- Around line 635-662: Update update_source_status to reuse the existing
uuid_value helper for source_id instead of constructing a Value::Bytes manually.
Move the updated_at pinning comment from above update_source_status to
immediately above update_evidence_status so it documents both status writers.
In `@src/backend/services/analytics/src/domain/metric_definitions/validator.rs`:
- Around line 130-178: Update the state match around EvidenceRelation::parse so
a missing evidence_ref remains ValidationState::Unchecked, while a present but
invalid evidence_ref returns an appropriate ValidationState::Error with the
existing schema error code and emits a warning identifying the invalid
reference. Preserve the existing validation flow for valid evidence references
and observation relations.
In `@src/backend/services/analytics/src/domain/metric_drilldown/mod.rs`:
- Around line 11-14: Move EVIDENCE_QUERY_TIMEOUT_SECS,
EVIDENCE_QUERY_MEMORY_BYTES, EVIDENCE_QUERY_READ_BYTES, and
EVIDENCE_QUERY_RESULT_BYTES out of metric_drilldown and colocate them with the
consumer in metric_definitions/validator.rs, or a shared probe-budget module if
appropriate. Update the validator imports/references to use their new location
and leave metric_drilldown focused on database and capability concerns.
In `@src/backend/services/analytics/src/domain/metric_results/builder.rs`:
- Around line 766-808: Update
metric_result_wire_shape_is_flat_with_computation_tag to assert the serialized
selection fields from selection(...) are present, including metric_key, entity,
period, and filters as appropriate, and verify that the None drilldown field is
omitted from the JSON. Keep the existing computation, format, and scale
assertions unchanged.
🪄 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 Plus
Run ID: 443bf5fc-7c53-4a41-a80f-db4ff86a985c
📥 Commits
Reviewing files that changed from the base of the PR and between 2bb09fbafc759324da91905737691c7567815eb8 and 436ce21f054882022eae2591bbecd5163be36f99.
📒 Files selected for processing (20)
docs/components/backend/analytics/openapi.jsondocs/domain/metrics/specs/DESIGN.mdsrc/backend/services/analytics/src/api/metric_results.rssrc/backend/services/analytics/src/domain/metric_definitions/builtin.rssrc/backend/services/analytics/src/domain/metric_definitions/definition.rssrc/backend/services/analytics/src/domain/metric_definitions/listing.rssrc/backend/services/analytics/src/domain/metric_definitions/live_tests.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/seeds.rssrc/backend/services/analytics/src/domain/metric_definitions/test_fixture.rssrc/backend/services/analytics/src/domain/metric_definitions/validator.rssrc/backend/services/analytics/src/domain/metric_drilldown/mod.rssrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/dto.rssrc/backend/services/analytics/src/domain/metric_results/mod.rssrc/backend/services/analytics/src/domain/metric_results/validation.rssrc/backend/services/analytics/src/domain/mod.rssrc/backend/services/analytics/src/migration/m20260727_000001_metric_evidence.rssrc/backend/services/analytics/src/migration/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/services/analytics/src/domain/metric_results/mod.rs
- docs/domain/metrics/specs/DESIGN.md
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
436ce21 to
0862081
Compare
2 participants
Summary
POST /v1/metric-resultsandGET /v1/metric-definitionsStack
Merges bottom-up; each PR retargets to
mainas its parent lands.UI layer: constructorfabric/insight-front#226
Closes #2068
Validation
cargo test -p analyticsSummary by CodeRabbit
distinct_countcomputation variant.