feat(metrics): task delivery on unified metrics - #1794
Merged
Conversation
Bounded index metrics (efficiency clamps, accuracy folds) had no home for their final shaping short of a bespoke computation variant. Add an optional affine + clamp transform on every definition — clamp(m*x + b) — applied after aggregation in every view and to zero-fills, keeping the computation vocabulary a closed algebra. The clamp is NULL-guarded: ClickHouse >=24.12 least/greatest ignore NULL arguments, which would resurrect an honest-null value as the bound and feed it to peer pools. Applied as an outer projection over column aliases, never inline, so it duplicates no aggregate placeholders. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Reproduce the task-delivery family as 14 tasks.* metrics over a new task_metric_observations gold view that reads class_task_* directly: per-issue state pivot, status-interval spans, and close/reopen transition pairing, all keyed on the source-neutral status_category. Metric-level decisions vs the legacy bullet path: - reopen_rate follows the spec — reopened within 14 days over close events, no minimum-close gate - resolution_time is a median (the legacy "mean" label was wrong) - flow_efficiency and worklog_accuracy keep their <=100 clamp, and estimation_accuracy its symmetric fold, via the value transform - closed/bugs/due/estimation count currently-done issues; duration metrics span every ever-closed issue Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
The task class tables alone dropped bronze's tenant_id. The unified gold view resolves an observation's tenant through the assignee identity join, so the directory that anchors that join must carry it; project it on both source staging models for a consistent union. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
|
Warning Review limit reached
Next review available in: 44 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 Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesTask metrics and unified value transforms
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TaskTracking
participant TaskModels
participant task_metric_observations
participant MetricDefinitions
participant QueryCompiler
participant ResultBuilder
TaskTracking->>TaskModels: provide task history, users, statuses, and worklogs
TaskModels->>task_metric_observations: materialize task measures
MetricDefinitions->>MetricDefinitions: persist and load ValueTransform
MetricDefinitions->>QueryCompiler: provide metric definition
QueryCompiler->>QueryCompiler: aggregate and transform metric values
QueryCompiler->>ResultBuilder: return transformed query results
ResultBuilder->>ResultBuilder: transform zero-filled values
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
changed the title
![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
🧹 Nitpick comments (2)
src/backend/services/analytics/src/domain/metric_results/compiler.rs (2)
1133-1168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the histogram in the “every query shape” regression test.
The test currently covers timeseries, breakdown, period, and peer queries but omits the changed histogram path at Lines 220-246. Assert that histogram windows use a separately transformed event column.
🤖 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/compiler.rs` around lines 1133 - 1168, Extend the transform_wraps_every_query_shape test to compile the histogram query and verify its SQL uses a separately transformed event column within the histogram window. Include the histogram query in the parameter-placeholder count assertion, preserving the existing coverage for timeseries, breakdown, period, and peer queries.
220-246: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a separate transformed histogram column.
src/backend/services/analytics/src/domain/metric_results/compiler.rs#L220-L246: project the transformed event value under a distinct name, then computemin/maxfrom that alias instead of reusingvaluein the same SELECT. That removes ClickHouse alias-shadowing risk.src/backend/services/analytics/src/domain/metric_results/compiler.rs#L1133-L1168: add a transformed histogram assertion so this query shape stays covered.🤖 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/compiler.rs` around lines 220 - 246, The histogram query at src/backend/services/analytics/src/domain/metric_results/compiler.rs:220-246 must project the transformed event value under a distinct alias and calculate entity_lo/entity_hi from that alias, avoiding reuse of value in the same SELECT. Add a transformed-histogram assertion at src/backend/services/analytics/src/domain/metric_results/compiler.rs:1133-1168 to cover this query shape.
🤖 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/ingestion/gold/task_metric_observations.sql`:
- Around line 231-232: Align the numerator and denominator filters in the
estimation percentage calculation so both averages use only completed issues
with positive estimates and non-NULL spent time. Update the avgIf conditions in
the task metric query while preserving the existing nullIf division guard.
- Line 162: Update the interval-end calculation over e.evs so a final event
representing a reopened/current status uses now() rather than the historical
s.final_close_at. Preserve final_close_at for issues that remain closed,
ensuring the resulting end is not before the reopen span start and the
downstream interval filtering retains the in-progress interval.
---
Nitpick comments:
In `@src/backend/services/analytics/src/domain/metric_results/compiler.rs`:
- Around line 1133-1168: Extend the transform_wraps_every_query_shape test to
compile the histogram query and verify its SQL uses a separately transformed
event column within the histogram window. Include the histogram query in the
parameter-placeholder count assertion, preserving the existing coverage for
timeseries, breakdown, period, and peer queries.
- Around line 220-246: The histogram query at
src/backend/services/analytics/src/domain/metric_results/compiler.rs:220-246
must project the transformed event value under a distinct alias and calculate
entity_lo/entity_hi from that alias, avoiding reuse of value in the same SELECT.
Add a transformed-histogram assertion at
src/backend/services/analytics/src/domain/metric_results/compiler.rs:1133-1168
to cover this query shape.
🪄 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: 7a79a17e-1e63-47de-8a72-bde2d7882c2d
📒 Files selected for processing (18)
src/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/repository.rssrc/backend/services/analytics/src/domain/metric_definitions/seeds.rssrc/backend/services/analytics/src/domain/metric_results/batch.rssrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/compiler.rssrc/backend/services/analytics/src/domain/metric_results/validation.rssrc/backend/services/analytics/src/migration/m20260714_000001_metric_value_transform.rssrc/backend/services/analytics/src/migration/mod.rssrc/ingestion/connectors/task-tracking/jira/dbt/jira__task_users.sqlsrc/ingestion/connectors/task-tracking/youtrack/dbt/youtrack__task_users.sqlsrc/ingestion/dbt/tests/gold/assert_task_observations_entity_id_shape.sqlsrc/ingestion/dbt/tests/gold/assert_task_observations_nonnegative.sqlsrc/ingestion/dbt/tests/gold/assert_task_observations_subject_key_shape.sqlsrc/ingestion/dbt/tests/gold/assert_task_observations_unique_grain.sqlsrc/ingestion/gold/schema.ymlsrc/ingestion/gold/task_metric_observations.sql
The last status span ended at final_close_at regardless of current state, so a closed-then-reopened issue's current span ended before it started and the interval sanity filter dropped the row. That hid the reopen transition (reopened_within_14d missed every currently-reopened issue and changed retroactively on re-close) and froze live in-progress accrual. End the tail span per current state instead — at the close for done issues, at now() otherwise — and count only pre-close spans toward a close's dev/pickup time, so live rework accrues to the next close and never retroactively inflates one already reported. Estimation accuracy averaged estimates and spent time over different row sets: avgIf skips NULLs per column, so NULL-spent issues fed the estimate average but not the spent average. Pin both averages to the same set — closed issues with a positive estimate and logged time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
The histogram projected the transformed event value as `value`, shadowing the source column it reads in the same SELECT. Project it as `event_value` in its own CTE and derive entity bounds and bin math from that alias; cover the shape in the transform test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
ClickHouse re-inlines every WITH reference, so each of the eleven measure branches re-evaluated the per-issue reconstruction — the build read 70x the silver input and peaked over 5 GiB. Materialize the two expensive stages (task_issue_state: field-history pivot + attribution + close; task_status_intervals: status spans) as their own gold tables so a branch re-scan costs a small sorted read: ~16x fewer rows read, ~14x less memory, 6x faster, identical output. Add the query_settings resource caps the other observation models carry (1.5 GiB memory, 4 threads, external group-by/sort spill). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
The jira dbt scope intersected tag:silver with the jira downstream, so the gold task-delivery chain hanging off class_task_* was never selected — task metrics would refresh only on deploys, while every other connector's plain tag:<name>+ scope rebuilds its observation models each sync. Union in the gold slice of the jira downstream. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
union_by_tag discovers contributors at runtime and creates no dbt graph edges — those come from the depends_on hints on each class model, which listed jira only. A youtrack sync's name-derived tag:youtrack+ scope therefore built staging views and nothing downstream: class_task_* never absorbed youtrack rows on youtrack syncs, and a youtrack-only instance would never refresh task metrics at all. Hint the seven classes with youtrack staging (field history has no youtrack source yet), mirroring the multi-source hints on the collaboration classes, and correct the stale dbt_select note on the youtrack descriptor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
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/ingestion/gold/task_metric_observations.sql (1)
77-83: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
issue_factsgrouped by source.
issue_idis only source-scoped upstream. Withoutinsight_source_idin this grouping, matching IDs for the same tenant/entity/date across Jira or YouTrack sources collapse into one fact and mix interval totals.Proposed fix
SELECT s.tenant_id, s.entity_id, + s.insight_source_id, toDate(s.final_close_at) AS metric_date, s.issue_id, ... - GROUP BY s.tenant_id, s.entity_id, s.issue_id, toDate(s.final_close_at) + GROUP BY + s.tenant_id, + s.entity_id, + s.insight_source_id, + s.issue_id, + toDate(s.final_close_at)🤖 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 77 - 83, Update the GROUP BY in the issue_facts aggregation to include s.insight_source_id alongside the existing tenant, entity, issue, and close-date keys. Preserve the current joins and interval aggregation while ensuring issue facts remain distinct across insight sources.
🤖 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/ingestion/gold/task_issue_state.sql`:
- Around line 64-74: Replace the non-NULL-preserving conditional aggregates at
all three affected sites: use maxIfOrNull for final_close_at in
task_issue_state.sql, and minIfOrNull for pickup_seconds and reopened_14d in
task_metric_observations.sql. Preserve the existing conditions and
derived-measure logic so no-match cases remain NULL.
---
Outside diff comments:
In `@src/ingestion/gold/task_metric_observations.sql`:
- Around line 77-83: Update the GROUP BY in the issue_facts aggregation to
include s.insight_source_id alongside the existing tenant, entity, issue, and
close-date keys. Preserve the current joins and interval aggregation while
ensuring issue facts remain distinct across insight sources.
🪄 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: d13a2e51-10c4-465c-9c75-774bfee795a3
📒 Files selected for processing (16)
src/backend/services/analytics/src/domain/metric_definitions/definition.rssrc/backend/services/analytics/src/domain/metric_results/compiler.rssrc/ingestion/connectors/task-tracking/jira/dbt/jira__task_users.sqlsrc/ingestion/connectors/task-tracking/jira/descriptor.yamlsrc/ingestion/connectors/task-tracking/youtrack/descriptor.yamlsrc/ingestion/gold/schema.ymlsrc/ingestion/gold/task_issue_state.sqlsrc/ingestion/gold/task_metric_observations.sqlsrc/ingestion/gold/task_status_intervals.sqlsrc/ingestion/silver/task-tracking/class_task_comments.sqlsrc/ingestion/silver/task-tracking/class_task_field_metadata.sqlsrc/ingestion/silver/task-tracking/class_task_projects.sqlsrc/ingestion/silver/task-tracking/class_task_sprints.sqlsrc/ingestion/silver/task-tracking/class_task_statuses.sqlsrc/ingestion/silver/task-tracking/class_task_users.sqlsrc/ingestion/silver/task-tracking/class_task_worklogs.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ingestion/connectors/task-tracking/jira/dbt/jira__task_users.sql
- src/backend/services/analytics/src/domain/metric_definitions/definition.rs
- src/backend/services/analytics/src/domain/metric_results/compiler.rs
aleksdotbar
changed the title
aleksdotbar
changed the title
maxIf over the non-Nullable event_at returns the epoch default, not NULL, when an issue has no done-category transition — so 3007 of 6742 issues carried a 1970 close time and passed the `final_close_at IS NOT NULL` gate that marks an issue closed. Output was unaffected (the is_done and duration gates masked them downstream) but the phantom closes are semantically wrong and processed for nothing. maxIfOrNull yields NULL on no match. Verified: every measure count/sum unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
ktursunov
approved these changes
Jul 17, 2026
The task-users staging views gained tenant_id mid-SELECT and class_task_users inherits its column order from that union, so the positional incremental insert breaks on any instance whose table predates the column. Add the guarded ADD/MODIFY AFTER heal to the migrate hook; existing rows carry NULL tenant until the next sync re-emits every user row. Staging needs no heal (both members are views). Verified by simulating the pre-change table: heal converges the schema and the positional insert succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
On a fresh instance the deploy-time tag:gold build runs against the silver placeholders (no staging is materialised yet, so the drop hook leaves them standing), and the task observation chain reads tenant_id from class_task_users — absent from the placeholder, the first deploy would fail at the gold build. Verified by building all three compiled gold models against a placeholder-schema database. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar
enabled auto-merge
July 17, 2026 09:39
ktursunov
approved these changes
Jul 17, 2026
This was referenced Jul 20, 2026
Closed
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports the task-delivery metric family to
/v1/metric-results, the lastIC family still on the legacy bullet path.
task_metric_observationsview reproduces per-issue state,status intervals, close/reopen (14d), worklog and estimation facts
from silver
class_task_*— 14tasks.*metrics registered inbuiltin.rs.ratios and folds render honest units without new computation kinds.
class_task_userscarriestenant_idfor the peer join.Entity gate is honest-absence: only email-shaped assignees rank; Jira
privacy hides the rest, matching git/collab.
Closes #1696
Summary by CodeRabbit