feat(metrics): add metric evidence drilldowns - #2003
Conversation
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar> # Conflicts: # src/ingestion/gold/collab_metric_observations.sql # src/ingestion/gold/git_metric_observations.sql
|
|
Warning Review limit reached
Next review available in: 31 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: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMetric evidence is added across analytics storage, ClickHouse gold models, API contracts, metric-result responses, drilldown query/export handlers, validation, and documentation. The new endpoints return typed paginated evidence and bounded CSV/XLSX exports. ChangesMetric evidence drilldown
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (16)
src/backend/services/analytics/src/api/metric_results.rs (1)
36-44: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun capability loading concurrently with the result queries.
capabilitiesis a lazy future and is first awaited after the ranking/views streams finish, so the initial DB lookup is serialized. Start it withjoin!/a spawned task or await it before result queries, then remove the misleadingtokio::pin!.🤖 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/api/metric_results.rs` around lines 36 - 44, The capability lookup created by load_capabilities must begin before the ranking/views result queries execute, rather than remaining lazy behind tokio::pin!. Update the surrounding metric-results flow to start capabilities concurrently using join! or a spawned task, or await it before launching the result queries, and remove the unnecessary tokio::pin!(capabilities).src/backend/services/analytics/src/domain/metric_drilldown/mod.rs (1)
541-550: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDimension names are string-interpolated into SQL.
filter.dimensionis allowlisted viadefinition.allowed_dimension, so this is not currently exploitable, but the safety depends entirely on the definition catalog never containing a quote character. Binding the dimension name as a parameter (it is only compared, never used as an identifier) removes that coupling.♻️ Suggested change
- let _ = write!( - filter_sql, - " AND indexOf(evidence.dimensions.1, '{}') > 0 AND evidence.dimensions.2[indexOf(evidence.dimensions.1, '{}')] IN ({placeholders})", - filter.dimension, filter.dimension - ); + let _ = write!( + filter_sql, + " AND indexOf(evidence.dimensions.1, ?) > 0 AND evidence.dimensions.2[indexOf(evidence.dimensions.1, ?)] IN ({placeholders})" + ); + params.push(filter.dimension.clone()); + params.push(filter.dimension.clone()); params.extend(filter.values.iter().cloned());Note the same pattern exists in
compile_ratio_query(Lines 613-622).🤖 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 541 - 550, Update the filter SQL construction in the current drilldown query and in compile_ratio_query so filter.dimension is passed through bound query parameters rather than interpolated into the SQL string. Add placeholders for both dimension comparisons, append the dimension value in matching parameter order, and preserve the existing filter.values parameter ordering and query behavior.src/backend/services/analytics/src/api/metric_drilldown.rs (1)
353-374: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
export_valuesruns twice per row for CSV exports.
ensure_export_input_boundmaterializes every cell, thenbuild_csvre-materializes them. At the 50k-row cap this doubles allocation/formatting work inside the blocking task while an export permit is held. Consider precomputing rows once (Vec<Vec<String>>) and passing that to both the bound check and the 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/api/metric_drilldown.rs` around lines 353 - 374, Update the export flow around ensure_export_input_bound and build_csv so each row’s export_values result is computed once and reused for both byte-limit validation and CSV writing. Precompute the formatted rows as Vec<Vec<String>> within the existing blocking task, pass that collection to the bound-checking logic and writers, and remove the duplicate per-row materialization while preserving the current limit behavior.src/backend/services/analytics/src/domain/metric_definitions/repository.rs (2)
613-621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing
uuid_valuehelper.
source_evidence_granularitiesandupdate_definition_statusin this same file bind UUIDs viauuid_value(...); this new writer hand-rollsValue::Bytes(Some(Box::new(source_id.as_bytes().to_vec()))).♻️ Proposed cleanup
- Value::Bytes(Some(Box::new(source_id.as_bytes().to_vec()))), + uuid_value(source_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 613 - 621, Replace the hand-rolled source_id byte binding in the new writer with the existing uuid_value helper, matching the usage in source_evidence_granularities and update_definition_status. Preserve the surrounding status, error_code, and config_revision bindings unchanged.
605-625: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAdd a no-op trace for stale config pins.
metric_sources.updated_atisDATETIME(3), and the extra%fpadding fromDATE_FORMAT(updated_at, ...) %Y-%m-%d %H:%i:%s.%fmatches the row value for non-edited rows. The stale-pin path still affects 0 rows and returnsOk(()), so logrows_affected == 0alongside the source/config revision to make mid-sweep edits diagnosable.🤖 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 605 - 625, After the metric_sources UPDATE in the repository method, inspect the returned execution result’s rows_affected value and emit a trace when it is zero, including source_id and config_revision. Preserve the existing update, await/error propagation, and Ok(()) behavior while making stale config-pin no-ops diagnosable.src/backend/services/analytics/src/domain/metric_definitions/validator.rs (1)
413-445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
has_exact_columnsis not an exact-set check.It verifies every expected column exists with the expected type, but a table with additional columns still returns
Present. The name reads as "exact column set"; a one-line doc comment ("exact type match; extra columns tolerated") would prevent a future reader from relying on the stronger guarantee. It also duplicateshas_columnsalmost entirely — the only difference is the predicate builder, which could be a parameter.🤖 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 413 - 445, Clarify the contract of has_exact_columns with a doc comment stating that expected columns must match their types while extra table columns are tolerated; do not imply an exact-set guarantee. Avoid broader refactoring of has_columns unless needed to preserve this documented behavior.src/ingestion/gold/ai_metric_observations.sql (1)
8-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
query_settingsfor a passthrough model.The model is now a plain projection over
ai_metric_evidence, yet it keepsjoin_algorithm: 'grace_hash,hash'and the 3 GiBmax_memory_usage, whilecollab_metric_observations.sqlandgit_metric_observations.sqlwere trimmed to 1.5 GiB with no join setting. Aligning them keeps the gold models' resource budgets consistent.♻️ Suggested alignment
query_settings={ - 'max_memory_usage': 3221225472, + 'max_memory_usage': 1610612736, 'max_threads': 4, 'max_bytes_before_external_group_by': 805306368, - 'max_bytes_before_external_sort': 805306368, - 'join_algorithm': 'grace_hash,hash' + 'max_bytes_before_external_sort': 805306368 }🤖 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/gold/ai_metric_observations.sql` around lines 8 - 15, Update the query_settings for the passthrough model to remove the obsolete join_algorithm setting and reduce max_memory_usage from 3 GiB to 1.5 GiB, matching the resource configuration used by collab_metric_observations.sql and git_metric_observations.sql. Preserve the remaining settings.src/backend/services/analytics/src/domain/metric_definitions/builtin.rs (1)
73-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGranularity is now encoded twice: here and in the dbt
granularitycolumn.This mapping duplicates the
granularityexpressions insrc/ingestion/gold/*_metric_evidence.sql(e.g.ai_metric_evidence.sqlline 131,collab_metric_evidence.sqllines 298-302,task_metric_evidence.sqllines 221-225). They agree today, but they can drift silently since the drilldown presentation reads the seeded DB value while the table stores its own. A registry test asserting each(source_key, measure_key)pair here matches the materializedgranularity(or dropping one of the two representations) would pin the contract.🤖 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/builtin.rs` around lines 73 - 98, Prevent granularity drift between BuiltinSource::evidence_granularity and the dbt granularity expressions in the metric evidence models. Prefer removing this duplicate mapping and reusing the seeded/materialized granularity representation; if both representations must remain, add a registry test covering every (source_key, measure_key) pair and asserting it matches the materialized database value.src/backend/services/analytics/src/domain/metric_definitions/definition.rs (1)
278-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the relation-name parser with
ObservationRelation.
EvidenceRelation::parse/table_ref/source_refare a byte-for-byte copy ofObservationRelation(lines 244-276) apart from the suffix. A small private helper (e.g.fn parse_relation(value: &str, suffix: &str) -> Option<String>) keeps the two validators from drifting, since both feed directly into interpolated{database}.{table}SQL. The doc comments explaining the naming contract are also missing here.♻️ Suggested shape
fn parse_relation(value: &str, suffix: &str) -> Option<String> { let family = value.strip_suffix(suffix)?; let mut chars = family.chars(); let starts_alpha = chars.next().is_some_and(|c| c.is_ascii_lowercase()); let rest_ok = family .chars() .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_'); (starts_alpha && rest_ok).then(|| value.to_owned()) }impl EvidenceRelation { pub const DATABASE: &'static str = "insight"; + /// Accepts exactly the managed-evidence naming shape: lowercase + /// `snake_case` ending in `_metric_evidence`, with a non-empty family + /// prefix. Anything else is a configuration error. pub fn parse(value: &str) -> Option<Self> { - let family = value.strip_suffix("_metric_evidence")?; - if family.is_empty() { - return None; - } - let starts_alpha = family - .chars() - .next() - .is_some_and(|c| c.is_ascii_lowercase()); - let rest_ok = family - .chars() - .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_'); - (starts_alpha && rest_ok).then(|| Self(value.to_owned())) + parse_relation(value, "_metric_evidence").map(Self) }Note the empty-family check is already implied by
starts_alphain both implementations.🤖 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/definition.rs` around lines 278 - 303, Extract the shared validation logic from ObservationRelation::parse and EvidenceRelation::parse into a private parse_relation helper accepting the value and suffix, and remove the redundant empty-family check because starts_alpha rejects empty names. Update both parsers to use the helper while preserving their suffixes and return types, and add the missing naming-contract documentation to EvidenceRelation’s parse/table_ref/source_ref methods consistent with ObservationRelation.src/ingestion/gold/task_metric_evidence.sql (3)
157-169: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
today()makes this model's output build-time dependent.
stale_in_progressis stamped with the run date rather than an event date, so re-running on a different day produces different rows and backfills are not reproducible. Worth documenting, or deriving the date fromlast_status_event_atif the intent is a point-in-time snapshot.🤖 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/gold/task_metric_evidence.sql` around lines 157 - 169, Update the stale CTE’s metric_date assignment to derive a deterministic date from s.last_status_event_at instead of today(), preserving reproducible results across reruns and backfills; retain the existing stale filtering and grouping behavior.
205-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead granularity branch.
value_measures(Lines 170-204) emits none ofdev_time_hours,resolution_days,pickup_days, so this arm always resolves to'derived_population'. Same shape assrc/ingestion/gold/git_metric_evidence.sqlLines 276-280; see the consolidated note below.🤖 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/gold/task_metric_evidence.sql` around lines 205 - 234, The granularity expression in the value_measures SELECT contains an unreachable event branch. Update the measure_key-based granularity logic to emit only the applicable 'derived_population' value, matching the corresponding git_metric_evidence pattern while preserving the rest of the projected columns.
60-96: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a uniqueness assertion for
(insight_source_id, issue_id).
task_issue_stategroups by issue at two subqueries, but the final select depends onclass_task_statusesandtask_usersalso being unique-per-user/source-status. Add a schema/assertion target for the resolved grain so duplicates don’t silently double-count evidence afterARRAY JOIN.🤖 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/gold/task_metric_evidence.sql` around lines 60 - 96, Add a schema/assertion target for issue_item_evidence that enforces uniqueness of the resolved (insight_source_id, issue_id) grain after class_task_statuses and task_users are applied. Ensure duplicate rows fail validation before ARRAY JOIN evidence can be emitted and double-counted.src/ingestion/gold/task_metric_observations.sql (1)
8-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
query_settings: this model no longer joins.The git counterpart got the same rewrite and had
join_algorithmdropped andmax_memory_usagehalved (src/ingestion/gold/git_metric_observations.sqlLines 9-13). This model is now a plain aggregate overtask_metric_evidence, so the join hint is inert and the 3 GB budget is unnecessarily large.♻️ Align with the git model
query_settings={ - 'max_memory_usage': 3221225472, + 'max_memory_usage': 1610612736, 'max_threads': 4, 'max_bytes_before_external_group_by': 805306368, - 'max_bytes_before_external_sort': 805306368, - 'join_algorithm': 'grace_hash,hash' + 'max_bytes_before_external_sort': 805306368 }🤖 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/gold/task_metric_observations.sql` around lines 8 - 15, Update the query_settings block in the task metric observations model to match the rewritten git metric model: remove the inert join_algorithm setting and reduce max_memory_usage from 3221225472 to the corresponding halved value, while preserving the remaining settings.src/ingestion/gold/git_metric_evidence.sql (3)
276-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead branch:
measure_observationsnever yields those measure keys.
measure_observations(Lines 245-259) only emitscommit_day,code_lines_added,lines_added,lines_removed. TheIN ('commit_change_size', 'pr_cycle_hours', 'pr_change_size')test is always false here, so this branch always resolves to'source_summary'. Consider collapsing it to the literal to avoid implying event granularity is reachable in this arm.♻️ Suggested simplification
- if( - measure_key IN ('commit_change_size', 'pr_cycle_hours', 'pr_change_size'), - 'event', - 'source_summary' - ) AS granularity, + 'source_summary' AS granularity,🤖 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/gold/git_metric_evidence.sql` around lines 276 - 280, In the granularity expression for the measure_observations output, replace the unreachable measure_key conditional with the literal 'source_summary'. Keep the existing measure_observations keys and surrounding query logic unchanged.
276-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEvent measure-key lists are hardcoded in four places with no single source of truth. The set that decides
granularity = 'event'in the evidence models is the same set that decides sum-vs-pass-through in the observations models. Adding an event measure to one side without the other silently changes aggregation semantics (an event measure left out of the observationsINlist gets summed per day; one left out of the evidencegranularityexpression is labelledsource_summary/derived_populationand the drilldown presentation insrc/backend/services/analytics/src/domain/metric_drilldown/mod.rsflipsshow_value). Extract each source's list into a dbt macro or var and reference it from all four sites.
src/ingestion/gold/git_metric_evidence.sql#L276-L280: this arm reads onlymeasure_observations, which emits none of the three keys, so the conditional is dead — replace it with the macro/var call (or the'source_summary'literal) so the list lives in one place.src/ingestion/gold/git_metric_observations.sql#L28-L45: replace both theNOT INandINliterals with the shared git event-measure list.src/ingestion/gold/task_metric_evidence.sql#L205-L234: this arm reads onlyvalue_measures, which emits none ofdev_time_hours/resolution_days/pickup_days, so the conditional is likewise dead — reference the shared task list instead.src/ingestion/gold/task_metric_observations.sql#L34-L46: replace both theNOT INandINliterals with the shared task event-measure list.🤖 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/gold/git_metric_evidence.sql` around lines 276 - 280, Define shared dbt macros or vars for the git and task event-measure lists, then reuse them at all four affected sites: src/ingestion/gold/git_metric_evidence.sql#L276-L280 must reference the git list in its granularity condition; src/ingestion/gold/git_metric_observations.sql#L28-L45 must use it for both NOT IN and IN checks; src/ingestion/gold/task_metric_evidence.sql#L205-L234 must reference the task list in its granularity condition; and src/ingestion/gold/task_metric_observations.sql#L34-L46 must use it for both NOT IN and IN checks. Keep each source’s event keys defined in only one place.
382-386: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueMissing null guards on the PR branches.
The
measure_observationsand commits arms both end withWHERE tenant_id IS NOT NULL AND entity_id IS NOT NULL AND metric_date IS NOT NULLbeforeassumeNotNull(...), but the two PR arms (Lines 352-357 and 382-386) rely only on the upstream CTE filters.entity_id/metric_dateare covered there,tenant_idis not. Adding the same guard keepsassumeNotNull(tenant_id)safe and consistent across all four arms.🤖 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/gold/git_metric_evidence.sql` around lines 382 - 386, Update both PR arms, including the branch using pr_measure from prs_merged_source, to add WHERE tenant_id IS NOT NULL alongside the existing entity_id and metric_date guards before assumeNotNull(tenant_id). Keep the current upstream filters and PR measurement logic unchanged.
🤖 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 `@docs/domain/metrics/README.md`:
- Line 74: Update the “Drilldown runtime and endpoints” documentation row to
also link the endpoint implementation at api/metric_drilldown.rs, or split the
runtime and endpoint references into separate rows while preserving the existing
domain-module link.
In `@src/backend/services/analytics/src/api/metric_drilldown.rs`:
- Around line 267-279: Update the Number branch in the metric drilldown
worksheet-writing match to handle values where as_f64() returns None by writing
the original value as text instead of propagating export_internal(). Preserve
numeric writing for parseable values and existing null handling.
In `@src/backend/services/analytics/src/api/metric_results.rs`:
- Around line 97-98: Only expose drilldown metadata when using the capability
that validates evidence-plan compatibility: update metric result population
around build_metric_result in
src/backend/services/analytics/src/api/metric_results.rs lines 97-98, apply the
same plan-compatible capability when populating listing metadata in
src/backend/services/analytics/src/domain/metric_definitions/listing.rs lines
107-110, and clarify inherited support in docs/domain/metrics/README.md lines
31-36 to require compatible inputs backed by the same evidence relation.
In `@src/backend/services/analytics/src/domain/metric_definitions/validator.rs`:
- Around line 464-497: Bound both evidence granularity probes in the validator
query flow around the evidence and observation SQL: reuse the drilldown query
settings for max execution time, memory, bytes read, and result bytes, and add a
recent metric_date predicate when the required window is available. Apply
identical safeguards to both queries while preserving their existing
source_key/measure_key filtering and result mapping.
- Around line 582-601: The EVIDENCE_COLUMN_TYPES definitions rely on brittle
byte-for-byte ClickHouse type strings, especially for dimensions and nullable
fields. Update has_exact_columns and its callers to validate evidence columns
using robust type semantics or a higher-level provenance/creation-point
invariant, rather than matching system.columns.type literals; preserve strict
validation where the invariant requires it.
- Around line 121-161: Update the evidence validation state flow so missing or
unparsable evidence_ref produces unchecked rather than Error(Unknown), and skip
evidence validation entirely for SourceKind::CustomObservationSql. In the
evidence_granularities_match Ok(false) branch, preserve the fail-closed error
state but emit a tracing::warn! containing the offending measure and granularity
details. Use the existing validation-state and source-kind symbols rather than
adding new status representations.
In `@src/backend/services/analytics/src/domain/metric_drilldown/mod.rs`:
- Around line 1005-1013: Update selection_fingerprint to exclude snapshot_id,
then validate the snapshot ID carried in the decoded cursor envelope against the
current snapshot before continuing pagination. Route mismatches through
verify_evidence_snapshot so snapshot rebuilds return EVIDENCE_SNAPSHOT_EXPIRED
instead of decode_cursor producing INVALID for cursor, while retaining
fingerprint validation for tenant and selection.
In
`@src/backend/services/analytics/src/migration/m20260727_000001_metric_evidence.rs`:
- Around line 11-46: Update migration m20260727_000001_metric_evidence to use
MySQL 8-compatible DDL: replace each ADD COLUMN IF NOT EXISTS with conditional
schema checks or plain ADD COLUMN statements appropriate for the migration
framework, and replace DROP CONSTRAINT IF EXISTS with MySQL-compatible DROP
CHECK using the named constraints. Preserve the existing column definitions,
ordering, and constraint expressions.
In `@src/ingestion/gold/git_metric_evidence.sql`:
- Around line 353-357: Update the pr_measure ARRAY JOIN expression to use a
null-safe change_size check, treating NULL as zero before evaluating whether it
is greater than 0, while preserving the existing metric tuple and empty-branch
behavior.
In `@src/ingestion/gold/schema.yml`:
- Around line 9-39: Add shared column tests to the evidence anchor
&metric_evidence_columns: apply not_null to the key columns, add accepted_values
for granularity with event, source_summary, and derived_population, and preserve
the existing accepted-value coverage for source_key, entity_type, and
measure_key. Add a build-integrity SQL test matching the pattern of
assert_metric_entity_cohorts_unique.sql to enforce uniqueness across tenant_id,
source_key, measure_key, entity_id, metric_date, and record_id.
In `@src/ingestion/gold/task_metric_evidence.sql`:
- Around line 238-257: The non-aggregated task evidence arms must exclude
unassigned entities before assumeNotNull is applied. In
src/ingestion/gold/task_metric_evidence.sql lines 238-257, add predicates for
tenant_id IS NOT NULL, entity_id IS NOT NULL, entity_id != '', and metric_date
IS NOT NULL after issue_item_evidence; apply the same guard in lines 261-285 for
the issue_facts arm, placing it so it filters rows produced by ARRAY JOIN.
- Around line 130-146: Update the close_reopen CTE’s GROUP BY to include
c.insight_source_id alongside the existing grouping keys, preserving the
composite (insight_source_id, issue_id) identity used by the closes/reopens
joins and preventing events from different sources from collapsing.
In `@src/ingestion/gold/task_worklog_flow.sql`:
- Around line 39-41: Update the day expansion around the ARRAY JOIN’s range()
expression to clamp the dateDiff result at zero before converting it to UInt32,
preventing inverted intervals from wrapping to a huge range. Also cap the
resulting day count to the established maximum span limit, if one exists, so
pathological live spans cannot generate unbounded rows.
- Around line 57-67: Update the FULL OUTER JOIN query using the visible
`ip`/`wl` join in `task_worklog_flow` to enable `join_use_nulls = 1` before the
`coalesce` expressions are evaluated. Preserve the existing coalesced key
selection and metric aggregation while ensuring unmatched sides remain NULL
rather than type defaults.
In `@src/ingestion/gold/wiki_metric_evidence.sql`:
- Around line 116-135: Update the page_creations event branch to exclude rows
with NULL tenant_id before the existing assumeNotNull(tenant_id) projection,
matching the summary branch’s guard behavior. Preserve the existing author_email
and created_at conditions and all other event-row mappings.
---
Nitpick comments:
In `@src/backend/services/analytics/src/api/metric_drilldown.rs`:
- Around line 353-374: Update the export flow around ensure_export_input_bound
and build_csv so each row’s export_values result is computed once and reused for
both byte-limit validation and CSV writing. Precompute the formatted rows as
Vec<Vec<String>> within the existing blocking task, pass that collection to the
bound-checking logic and writers, and remove the duplicate per-row
materialization while preserving the current limit behavior.
In `@src/backend/services/analytics/src/api/metric_results.rs`:
- Around line 36-44: The capability lookup created by load_capabilities must
begin before the ranking/views result queries execute, rather than remaining
lazy behind tokio::pin!. Update the surrounding metric-results flow to start
capabilities concurrently using join! or a spawned task, or await it before
launching the result queries, and remove the unnecessary
tokio::pin!(capabilities).
In `@src/backend/services/analytics/src/domain/metric_definitions/builtin.rs`:
- Around line 73-98: Prevent granularity drift between
BuiltinSource::evidence_granularity and the dbt granularity expressions in the
metric evidence models. Prefer removing this duplicate mapping and reusing the
seeded/materialized granularity representation; if both representations must
remain, add a registry test covering every (source_key, measure_key) pair and
asserting it matches the materialized database value.
In `@src/backend/services/analytics/src/domain/metric_definitions/definition.rs`:
- Around line 278-303: Extract the shared validation logic from
ObservationRelation::parse and EvidenceRelation::parse into a private
parse_relation helper accepting the value and suffix, and remove the redundant
empty-family check because starts_alpha rejects empty names. Update both parsers
to use the helper while preserving their suffixes and return types, and add the
missing naming-contract documentation to EvidenceRelation’s
parse/table_ref/source_ref methods consistent with ObservationRelation.
In `@src/backend/services/analytics/src/domain/metric_definitions/repository.rs`:
- Around line 613-621: Replace the hand-rolled source_id byte binding in the new
writer with the existing uuid_value helper, matching the usage in
source_evidence_granularities and update_definition_status. Preserve the
surrounding status, error_code, and config_revision bindings unchanged.
- Around line 605-625: After the metric_sources UPDATE in the repository method,
inspect the returned execution result’s rows_affected value and emit a trace
when it is zero, including source_id and config_revision. Preserve the existing
update, await/error propagation, and Ok(()) behavior while making stale
config-pin no-ops diagnosable.
In `@src/backend/services/analytics/src/domain/metric_definitions/validator.rs`:
- Around line 413-445: Clarify the contract of has_exact_columns with a doc
comment stating that expected columns must match their types while extra table
columns are tolerated; do not imply an exact-set guarantee. Avoid broader
refactoring of has_columns unless needed to preserve this documented behavior.
In `@src/backend/services/analytics/src/domain/metric_drilldown/mod.rs`:
- Around line 541-550: Update the filter SQL construction in the current
drilldown query and in compile_ratio_query so filter.dimension is passed through
bound query parameters rather than interpolated into the SQL string. Add
placeholders for both dimension comparisons, append the dimension value in
matching parameter order, and preserve the existing filter.values parameter
ordering and query behavior.
In `@src/ingestion/gold/ai_metric_observations.sql`:
- Around line 8-15: Update the query_settings for the passthrough model to
remove the obsolete join_algorithm setting and reduce max_memory_usage from 3
GiB to 1.5 GiB, matching the resource configuration used by
collab_metric_observations.sql and git_metric_observations.sql. Preserve the
remaining settings.
In `@src/ingestion/gold/git_metric_evidence.sql`:
- Around line 276-280: In the granularity expression for the
measure_observations output, replace the unreachable measure_key conditional
with the literal 'source_summary'. Keep the existing measure_observations keys
and surrounding query logic unchanged.
- Around line 276-280: Define shared dbt macros or vars for the git and task
event-measure lists, then reuse them at all four affected sites:
src/ingestion/gold/git_metric_evidence.sql#L276-L280 must reference the git list
in its granularity condition;
src/ingestion/gold/git_metric_observations.sql#L28-L45 must use it for both NOT
IN and IN checks; src/ingestion/gold/task_metric_evidence.sql#L205-L234 must
reference the task list in its granularity condition; and
src/ingestion/gold/task_metric_observations.sql#L34-L46 must use it for both NOT
IN and IN checks. Keep each source’s event keys defined in only one place.
- Around line 382-386: Update both PR arms, including the branch using
pr_measure from prs_merged_source, to add WHERE tenant_id IS NOT NULL alongside
the existing entity_id and metric_date guards before assumeNotNull(tenant_id).
Keep the current upstream filters and PR measurement logic unchanged.
In `@src/ingestion/gold/task_metric_evidence.sql`:
- Around line 157-169: Update the stale CTE’s metric_date assignment to derive a
deterministic date from s.last_status_event_at instead of today(), preserving
reproducible results across reruns and backfills; retain the existing stale
filtering and grouping behavior.
- Around line 205-234: The granularity expression in the value_measures SELECT
contains an unreachable event branch. Update the measure_key-based granularity
logic to emit only the applicable 'derived_population' value, matching the
corresponding git_metric_evidence pattern while preserving the rest of the
projected columns.
- Around line 60-96: Add a schema/assertion target for issue_item_evidence that
enforces uniqueness of the resolved (insight_source_id, issue_id) grain after
class_task_statuses and task_users are applied. Ensure duplicate rows fail
validation before ARRAY JOIN evidence can be emitted and double-counted.
In `@src/ingestion/gold/task_metric_observations.sql`:
- Around line 8-15: Update the query_settings block in the task metric
observations model to match the rewritten git metric model: remove the inert
join_algorithm setting and reduce max_memory_usage from 3221225472 to the
corresponding halved value, while preserving the remaining settings.
🪄 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: a9d9b915-c002-4f75-b914-7b99a0e3ba6b
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
docs/components/backend/analytics/openapi.jsondocs/domain/metrics/README.mddocs/domain/metrics/specs/DESIGN.mdsrc/backend/Cargo.tomlsrc/backend/services/analytics/Cargo.tomlsrc/backend/services/analytics/src/api/metric_drilldown.rssrc/backend/services/analytics/src/api/metric_results.rssrc/backend/services/analytics/src/api/mod.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/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/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.rssrc/ingestion/gold/ai_metric_evidence.sqlsrc/ingestion/gold/ai_metric_observations.sqlsrc/ingestion/gold/collab_metric_evidence.sqlsrc/ingestion/gold/collab_metric_observations.sqlsrc/ingestion/gold/git_metric_evidence.sqlsrc/ingestion/gold/git_metric_observations.sqlsrc/ingestion/gold/schema.ymlsrc/ingestion/gold/task_metric_evidence.sqlsrc/ingestion/gold/task_metric_observations.sqlsrc/ingestion/gold/task_worklog_flow.sqlsrc/ingestion/gold/wiki_metric_evidence.sqlsrc/ingestion/gold/wiki_metric_observations.sql
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
There was a problem hiding this comment.
Actionable comments posted: 3
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/migration/m20260727_000001_metric_evidence.rs (1)
108-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate both sources when
source_idchanges.This trigger only checks
evidence_granularityand updatesNEW.source_id. Reassigning a measure from one source to another without changing its granularity leaves the old source’s evidence status stale and does not invalidate the new source. Includesource_idin the change predicate and invalidate bothOLD.source_idandNEW.source_id(or enforce source immutability explicitly).Proposed fix
- IF NOT (OLD.evidence_granularity <=> NEW.evidence_granularity) THEN + IF NOT (OLD.evidence_granularity <=> NEW.evidence_granularity) + OR NOT (OLD.source_id <=> NEW.source_id) THEN UPDATE metric_sources SET evidence_schema_status = 'unchecked', evidence_schema_checked_at = NULL, evidence_schema_error_code = NULL - WHERE id = NEW.source_id; + WHERE id = OLD.source_id + OR id = NEW.source_id; END IF;🤖 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/migration/m20260727_000001_metric_evidence.rs` around lines 108 - 120, Update the trg_metric_source_measures_evidence_update trigger to detect changes to both evidence_granularity and source_id, then invalidate metric_sources for both OLD.source_id and NEW.source_id when either field changes. Preserve the existing reset values for evidence_schema_status, evidence_schema_checked_at, and evidence_schema_error_code.
🤖 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/api/http_live_tests.rs`:
- Around line 162-170: The metadata helpers enable_drilldown_metadata in
src/backend/services/analytics/src/api/http_live_tests.rs:162-170 and the
corresponding logic in
src/backend/services/analytics/src/domain/metric_definitions/live_tests.rs:165-179
must stop updating every row; limit mutations to test-owned fixtures and restore
each fixture’s prior status values during cleanup, using the same isolated
fixture/cleanup strategy at both sites.
In `@src/backend/services/analytics/src/domain/metric_definitions/live_tests.rs`:
- Around line 237-241: Update the test invoking
MetricDefinitionValidator::new(db, ch).validate_all() to inspect its result
instead of discarding it. Assert the expected outcome for the unavailable
ClickHouse client, including any expected error or status update, so the test
fails when validation handles the outage unexpectedly.
- Around line 212-227: Update the test around the two update_evidence_status
calls to read and assert the stored status after the current-revision write,
then perform the distinct stale-revision update and read the row again. Assert
the stale write leaves the previously stored value unchanged, ensuring the
revision predicate is exercised.
---
Outside diff comments:
In
`@src/backend/services/analytics/src/migration/m20260727_000001_metric_evidence.rs`:
- Around line 108-120: Update the trg_metric_source_measures_evidence_update
trigger to detect changes to both evidence_granularity and source_id, then
invalidate metric_sources for both OLD.source_id and NEW.source_id when either
field changes. Preserve the existing reset values for evidence_schema_status,
evidence_schema_checked_at, and evidence_schema_error_code.
🪄 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: 78494da4-0c0d-4d6c-ba56-495cd0e06d28
📒 Files selected for processing (20)
docs/domain/metrics/README.mdsrc/backend/services/analytics/src/api/http_live_tests.rssrc/backend/services/analytics/src/api/metric_drilldown.rssrc/backend/services/analytics/src/api/metric_results.rssrc/backend/services/analytics/src/domain/metric_definitions/definition.rssrc/backend/services/analytics/src/domain/metric_definitions/live_tests.rssrc/backend/services/analytics/src/domain/metric_definitions/repository.rssrc/backend/services/analytics/src/domain/metric_definitions/validator.rssrc/backend/services/analytics/src/domain/metric_drilldown/mod.rssrc/backend/services/analytics/src/migration/m20260727_000001_metric_evidence.rssrc/ingestion/dbt/tests/gold/assert_metric_evidence_unique.sqlsrc/ingestion/dbt/tests/gold/assert_task_issue_state_unique.sqlsrc/ingestion/gold/ai_metric_observations.sqlsrc/ingestion/gold/git_metric_evidence.sqlsrc/ingestion/gold/schema.ymlsrc/ingestion/gold/task_metric_evidence.sqlsrc/ingestion/gold/task_metric_observations.sqlsrc/ingestion/gold/task_worklog_flow.sqlsrc/ingestion/gold/wiki_metric_evidence.sqlsrc/ingestion/tests/e2e/api/test_metric_drilldown.py
🚧 Files skipped from review as they are similar to previous changes (14)
- src/ingestion/gold/task_worklog_flow.sql
- src/ingestion/gold/ai_metric_observations.sql
- src/ingestion/gold/task_metric_observations.sql
- docs/domain/metrics/README.md
- src/ingestion/gold/wiki_metric_evidence.sql
- src/ingestion/gold/git_metric_evidence.sql
- src/ingestion/gold/schema.yml
- src/ingestion/gold/task_metric_evidence.sql
- src/backend/services/analytics/src/domain/metric_definitions/definition.rs
- src/backend/services/analytics/src/domain/metric_definitions/validator.rs
- src/backend/services/analytics/src/api/metric_results.rs
- src/backend/services/analytics/src/api/metric_drilldown.rs
- src/backend/services/analytics/src/domain/metric_definitions/repository.rs
- src/backend/services/analytics/src/domain/metric_drilldown/mod.rs
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
1 participant
Summary
Validation
cargo check -p analyticsdbt parsedbt run --select tag:goldSummary by CodeRabbit