Skip to content

feat: migrate git metrics to unified metric system (backend) - #1706

Merged
aleksdotbar merged 13 commits into
mainfrom
feat/unified-metrics-git
Jul 10, 2026
Merged

feat: migrate git metrics to unified metric system (backend)#1706
aleksdotbar merged 13 commits into
mainfrom
feat/unified-metrics-git

Conversation

@aleksdotbar

@aleksdotbar aleksdotbar commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Migrate git metric family to unified metrics, served through POST /v1/metric-results. Adds the git runtime vocab, a dbt gold observation model, and registry seeds.

Closes #1694. Part of #1561. Builds on #1681.

Additive, safe ahead of the frontend: legacy git gold views (git_bullet_rows, ic_kpis, ic_chart_loc) and catalog seeds untouched. Legacy path retires after frontend cutover (separate task).

Changes

  • ObservationSource enum → ObservationRelation newtype. Relation name is data in metric_sources.source_ref, validated against the <family>_metric_observations shape on load, runtime-gated by the schema probe. New source = dbt model + registry rows, no code. No DB migration.
  • median computation — quantileExact(0.5) over per-event values (new event_measure dbt macro). Single + batched paths. Peer view swaps only the per-entity aggregate; cohort scoping, disclosure floor, honest-null unchanged. Never zero-filled.
  • histogram view — ten server-owned fixed-width bins over an entity's exact [min, max], last bin closed, equal values collapse, event-less entities empty. Deterministic, not the adaptive histogram(). Median metrics only. SQL owns membership, builder owns edges.
  • Forward-only migration widens the computation_type enum, re-adds CHECK same name (idempotent DROP CONSTRAINT IF EXISTS).
  • insight.git_metric_observations gold view over the class contracts (not legacy fct_/mtr_) + shared file-classification macro. Ten git.* definitions.

Metrics (source git)

metric computation notes
git.commits sum distinct non-merge commit hashes
git.code_lines sum lines added to code-category files
git.lines_added sum all lines added, split by file category
git.prs_created sum
git.prs_merged sum merge-dated throughput
git.merge_rate ratio share of PRs created in the period that merged (created-cohort)
git.commits_per_active_day ratio
git.commit_size median typical commit diff size
git.pr_size median typical PR diff size
git.pr_cycle_time_h median typical open→merge hours

Dimensions: source (github/gitlab/bitbucket_cloud), category (code/test/config/docs). File class computed in the gold view via a shared macro → taxonomy changes apply retroactively.

PR author attribution

PR rows often lack an author email. Two tiers, most-precise first:

  1. Direct author_email (e.g. GitLab, from the users stream).
  2. Per-PR commit crosswalk — dominant author email among the PR's linked commits (tie → unresolved). commit_hash join, account-agnostic, carries the bulk.

Unresolved excluded, never misattributed. Account-handle-keyed resolution is a follow-up.

Deltas vs legacy

  • merge_rate created-cohort: numerator and denominator both created-dated → bounded to 100% (legacy mixed a merge-dated numerator with a create-dated denominator, could exceed 100%).
  • commit_size median, not the legacy mean — outlier-robust, symmetric with pr_size.
  • cycle time / PR size medians (legacy label said "average"; SQL already computed a median).
  • Merge commits excluded from commit measures and PR identity election.
  • docs split out of what legacy counted as code; legacy spec label corrected to test (matches its patterns).

Verified

  • Adversarial review, multiple passes; confirmed findings fixed (migration idempotency, merge-rate cohort, entity-id parity, distinct-commit vote, merge-commit exclusion).
  • cargo test + clippy clean; migration CHECK-name pinned by test.
  • dbt build green incl. integrity tests (entity-id shape, dimension vocabs, non-negativity).
  • End-to-end vs a local warehouse: all metrics × all views, error cases, legacy-vs-unified parity.

Follow-ups

  • Frontend git-group cutover.
  • Retire legacy git gold views + catalog seeds.
  • Account-handle-keyed PR identity resolution.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added median-based metric calculations, including commit size, pull request size, and cycle time metrics.
    • Added histogram views for median metrics with 10 deterministic bins per entity.
    • Added unified Git metrics covering commits, pull requests, file changes, categories, and data sources.
    • Added clearer Git labels for code, tests, configuration, documentation, and supported providers.
  • Bug Fixes

    • Improved validation for metric sources, unsupported histogram requests, and oversized result requests.
    • Median metrics with no observations now return an empty value instead of zero.

aleksdotbar and others added 7 commits July 8, 2026 16:04
The runtime resolved observation tables through a single-variant
ObservationSource enum, so registering a new managed source meant a new
enum variant plus table-name plumbing. Replace it with an
ObservationRelation newtype: metric_sources.source_ref already stores
the relation name; parse it on every load against the
<family>_metric_observations naming shape and let the existing schema
probe gate availability per relation. Adding an observation source is
now a dbt gold model plus registry seed rows.

The compile-time enum bought no real safety — the warehouse schema is a
runtime fact and the column probe was always the effective gate. The
newtype keeps parse-don't-validate discipline without freezing the
source list into code.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Two vocabulary extensions the git metric family needs.

median: quantileExact(0.5) over per-event observation values (one row
per source event, emitted by the new event_measure dbt shape macro).
Folds into the existing single-measure predicate and param layout; the
peer view swaps only the per-entity aggregate, so cohort scoping,
observed-only pools, and the disclosure floor apply unchanged. Medians
join ratios in honest-null: no rows means null, never a fabricated
zero. The computation_type enum and CHECK constraint extend under the
same constraint name so the startup probe stays green.

histogram: a new result view that bins one entity's own per-event
values within the period — the shape a single number cannot show.
Valid only for median metrics. Bins are server-owned and deterministic:
ten fixed-width bins over the entity's exact [min, max], last bin
closed, identical values collapse to one bin, entities without events
stay listed with empty bins. SQL owns bin membership; the builder owns
every edge, so empty and observed bins can never disagree. Projected
row math counts entities x bins.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Adds the git source end to end: gold observations, identity bridge,
file taxonomy, registry seeds, and fresh-cluster placeholders.

insight.git_metric_observations reads the class contracts only —
fct_git_*/mtr_git_* stay legacy-serving. Day-grain sums cover commits,
code lines, category-split lines added, and PR counts; event-grain
measures (commit and PR diff sizes, PR cycle hours) feed the median
metrics. File classification (code/test/config/docs) is computed in the
gold view through one shared macro so taxonomy changes apply
retroactively — the docs category also stops README-style files
counting as clean code, and the former 'spec' naming is corrected to
what its patterns always matched: tests.

Pull-request authorship resolves in tiers and never guesses: the PR's
own email, else the dominant author email among the PR's linked
commits, else identity.git_actor_emails — a bridge view electing the
dominant commit email per actor name with directory identity_inputs as
fallback; ties and unknowns resolve to exclusion, not misattribution.

Registry seeds ten git.* metrics (sums, two ratios, three medians) with
honest explanations: cycle time and sizes are medians, merge rate
carries its period-edge caveat, commit_size replaces the legacy
mean-based lines-per-commit with the outlier-robust statistic already
used for PR size. Build-integrity dbt tests pin bridge uniqueness,
entity id shape, dimension vocabularies, and non-negativity;
placeholder coverage extends to the PR-commits link table and
identity_inputs so the gold view type-checks on a fresh cluster.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
identity_inputs stores insight_tenant_id as UUID in the current model
but as String in older incremental tables; ClickHouse refuses mixed
UUID/String join keys. The bridge compares canonical string forms on
both sides, immune to the physical column type.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
MariaDB rejects `ALTER TABLE ... DROP CHECK` (MySQL 8 syntax); it
drops CHECK constraints through `DROP CONSTRAINT`. The median migration
failed on its first statement against MariaDB — caught running it live
against the local stack. Same constraint name, so the startup CHECK
probe is unaffected.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
… migration

Review follow-ups on the git family (no bridge changes):

- git.merge_rate becomes a created-cohort conversion. The numerator was
  merge-dated and the denominator create-dated, so a PR created before
  the window but merged inside it could push the rate over 100%. A new
  created-dated pr_created_merged measure (PRs created in the period
  that have merged) makes the numerator a subset of the denominator —
  bounded to 100% and read as 'share of created PRs that merged'.
  pr_merged stays merge-dated for the standalone throughput metric.

- Git entity ids trim as well as lowercase, matching the API's
  entity-id normalization; an untrimmed id would never match a request.

- The PR identity election votes by distinct linked commit rather than
  join row and excludes merge commits — consistent with the commit
  observations and immune to a hash present in multiple repos.

- The median-computation migration drops its CHECK with IF EXISTS, so a
  warm cluster whose constraint was already removed still reaches the
  enum widen instead of aborting startup.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
The name bridge is the last-resort fallback (tier 3) behind direct
author email and the per-PR dominant-commit-email crosswalk; these
tighten its precision without touching the tiers that carry the load.

- A commit co-occurrence tie now excludes the name instead of deferring
  to the directory. A tie is evidence the name is shared by two people;
  letting the directory's single mapping win would attribute both to
  one. Commit signal is authoritative when a name appears in commits at
  all, so only names with no commit signal fall back to the directory.

- Filter names/emails on their normalized (trimmed) form, so a
  whitespace-only value can't survive the non-empty guard and collide on
  the empty string.

- Emit trimmed lowercased emails, matching the API's entity-id
  normalization so a bridge-resolved id is never one a request misses.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
@aleksdotbar
aleksdotbar requested a review from a team as a code owner July 8, 2026 17:02
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds median computation and histogram result support, replaces enum-based observation sources with validated relation names, and introduces dbt-based git observation models, metric registrations, integrity tests, and silver placeholder schema updates.

Changes

Median & Histogram Backend

Layer / File(s) Summary
Metric contracts and observation relations
src/backend/services/analytics/src/domain/metric_definitions/{definition.rs,repository.rs,validator.rs,mod.rs}
Adds median computation contracts and replaces ObservationSource with validated ObservationRelation handling.
Builtin metrics and computation migration
src/backend/services/analytics/src/domain/metric_definitions/{builtin.rs,seeds.rs}, src/backend/services/analytics/src/migration/*
Registers git and median metrics, persists relation names, and widens the database computation enum.
Median SQL and histogram execution
src/backend/services/analytics/src/domain/metric_results/{compiler.rs,batch.rs,builder.rs}, src/backend/services/analytics/src/api/metric_results.rs
Compiles median and deterministic histogram queries, plans histogram execution, and builds result payloads.
Histogram DTOs and validation
src/backend/services/analytics/src/domain/metric_results/{dto.rs,view.rs,validation.rs}
Adds histogram request/response types, restricts histogram views to median metrics, and applies fixed-bin row limits.
Metric design specification
docs/domain/metrics/specs/DESIGN.md
Documents relation validation, median semantics, histogram binning, measure grain, and authoring rules.

Git Metrics Ingestion Pipeline

Layer / File(s) Summary
Git observation macros and gold model
src/ingestion/dbt/macros/*, src/ingestion/gold/git_metric_observations.sql
Adds git taxonomy and event-grain macros and produces unified commit, file-change, and pull-request observations.
Git observation contract and integrity tests
src/ingestion/gold/schema.yml, src/ingestion/dbt/tests/gold/*
Defines the git observation schema and validates dimensions, entity identifiers, and non-negative values.
Silver placeholders and ingestion documentation
src/ingestion/scripts/create-bronze-placeholders.sh, src/ingestion/silver/git/README.md
Updates placeholder schemas and documents the unified metrics ingestion path.

Estimated code review effort: 4 (Complex) | ~75 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: ktursunov, cyberantonz, mitasovr

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.30% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: migrating git metrics to the unified metric system.
Linked Issues check ✅ Passed The PR adds the git gold observation model, registry definitions, dimensions/cohorts mapping, and unit/dbt tests required by #1694.
Out of Scope Changes check ✅ Passed The changes stay within the backend git-metrics migration and supporting docs/tests, with no clear unrelated scope added.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/unified-metrics-git

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
src/ingestion/silver/git/git_actor_emails.sql (1)

1-19: 🚀 Performance & Scalability | 🔵 Trivial

Full-history view recomputed on every read of the identity bridge.

This view joins full commit/PR history plus identity_inputs FINAL on every read, and is itself joined by git_metric_observations for every query that resolves PR authors. The retroactivity rationale is sound, but as commit history grows this could become a meaningful cost on the metrics read path. Worth keeping an eye on materialization strategy (e.g. periodic snapshot table + incremental refresh) if query latency becomes an issue.

🤖 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/silver/git/git_actor_emails.sql` around lines 1 - 19, The
`git_actor_emails` view is being recomputed from full history on every read,
which can become expensive as commit volume grows. Review the materialization
approach in `git_actor_emails` and its downstream use in
`git_metric_observations`, and consider a snapshot or incremental-refresh
pattern instead of a pure view if read latency becomes a problem.
src/ingestion/gold/git_metric_observations.sql (1)

101-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nested aggregate-in-window is correct but non-obvious; consider using the alias.

max(uniqExact(commits.commit_hash)) OVER (...) (lines 122-125) re-invokes the exact aggregate expression already computed as email_count (line 121) instead of referencing that alias inside the window function. This likely works in ClickHouse (window functions apply over the grouped result and the identical aggregate expression is resolved consistently), but it's an unusual pattern worth double-checking under dbt build since window+GROUP BY interactions have had ClickHouse version-specific quirks. Using the email_count alias directly would be clearer and avoid any ambiguity about whether the aggregate is recomputed.

♻️ Suggested clarity fix
-            max(uniqExact(commits.commit_hash)) OVER (
+            max(email_count) OVER (
                 PARTITION BY links.tenant_id, links.source_id,
                              links.project_key, links.repo_slug, links.pr_id
             ) AS max_count
🤖 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_observations.sql` around lines 101 - 141, The
`pr_commit_emails` CTE is reusing the full `uniqExact(commits.commit_hash)`
expression inside the window instead of the already-computed `email_count`
alias, which is harder to read and may be brittle in ClickHouse. Update the
`max_count` calculation in `pr_commit_emails` to reference the grouped
`email_count` result directly (or otherwise factor the aggregate once), keeping
the same `PARTITION BY` over `links.tenant_id`, `links.source_id`,
`links.project_key`, `links.repo_slug`, and `links.pr_id`.
🤖 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/specs/DESIGN.md`:
- Around line 412-415: Case 3 still refers to the removed ObservationSource enum
and its from_ref/table_ref mapping, so update the instructions to match the
current ObservationRelation/source_ref model. In the DESIGN.md Case 3 section,
remove the step that tells developers to add an enum variant and table-name
mapping in definition.rs, and renumber the remaining steps so the guidance
reflects that adding a source is now just dbt plus registry seed changes. Also
align the referenced db_strings_round_trip test description with
ObservationRelation round-trips instead of ObservationSource pairs.

In `@src/ingestion/gold/git_metric_observations.sql`:
- Around line 8-14: The header contract in git_metric_observations.sql currently
says no vendor-specific label mappings may appear, but commits_source and
pull_requests_source both hardcode the same data_source-to-product label mapping
in two places. Fix this by either updating the header to match the implemented
behavior or, preferably, extracting the mapping into a shared macro like
git_file_category_label so the logic is centralized and the contract remains
accurate.
- Around line 47-77: The commits_source CTE currently uses LIMIT 1 BY without a
deterministic ORDER BY, so the surviving row for forked commits can vary across
runs and change the project_key/repo_slug used later in the file_changes_source
join. Update commits_source to sort deterministically on the identifying commit
fields before the LIMIT 1 BY clause, using the same symbols already present in
the CTE (such as tenant_id, data_source, commit_hash, project_key, and
repo_slug) so the chosen row is stable.

In `@src/ingestion/scripts/create-bronze-placeholders.sh`:
- Around line 666-689: The identity.identity_inputs placeholder is created in
create-bronze-placeholders.sh but is not covered by
drop_silver_placeholders_at_start, so it can linger and block the real model.
Update the cleanup path to also remove this placeholder, using the same
identity.identity_inputs symbol and the INSIGHT_PLACEHOLDER_v1 table marker,
either by extending the drop logic to include identity-tagged placeholders or by
adding an explicit drop for this table before the real build runs.

In `@src/ingestion/silver/git/git_actor_emails.sql`:
- Around line 131-161: The outer filter in git_actor_emails.sql is accidentally
coupled to the SELECT alias assumeNotNull(email), so the null/empty check may
resolve against the aliased value instead of the raw nullable column. Update the
query around the outer SELECT/WHERE to isolate the filter from the alias by
using a separate subquery or by renaming the projected email expression, and
make sure the WHERE condition in the final projection applies to the underlying
nullable email value from the inner query.

---

Nitpick comments:
In `@src/ingestion/gold/git_metric_observations.sql`:
- Around line 101-141: The `pr_commit_emails` CTE is reusing the full
`uniqExact(commits.commit_hash)` expression inside the window instead of the
already-computed `email_count` alias, which is harder to read and may be brittle
in ClickHouse. Update the `max_count` calculation in `pr_commit_emails` to
reference the grouped `email_count` result directly (or otherwise factor the
aggregate once), keeping the same `PARTITION BY` over `links.tenant_id`,
`links.source_id`, `links.project_key`, `links.repo_slug`, and `links.pr_id`.

In `@src/ingestion/silver/git/git_actor_emails.sql`:
- Around line 1-19: The `git_actor_emails` view is being recomputed from full
history on every read, which can become expensive as commit volume grows. Review
the materialization approach in `git_actor_emails` and its downstream use in
`git_metric_observations`, and consider a snapshot or incremental-refresh
pattern instead of a pure view if read latency becomes a problem.
🪄 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: 1120bfc2-43d9-4fc1-8028-28e7d08f0dad

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf4fef and c75208d.

📒 Files selected for processing (28)
  • docs/domain/metrics/specs/DESIGN.md
  • src/backend/services/analytics/src/api/metric_results.rs
  • src/backend/services/analytics/src/domain/metric_definitions/builtin.rs
  • src/backend/services/analytics/src/domain/metric_definitions/definition.rs
  • src/backend/services/analytics/src/domain/metric_definitions/mod.rs
  • src/backend/services/analytics/src/domain/metric_definitions/repository.rs
  • src/backend/services/analytics/src/domain/metric_definitions/seeds.rs
  • src/backend/services/analytics/src/domain/metric_definitions/validator.rs
  • src/backend/services/analytics/src/domain/metric_results/builder.rs
  • src/backend/services/analytics/src/domain/metric_results/compiler.rs
  • src/backend/services/analytics/src/domain/metric_results/dto.rs
  • src/backend/services/analytics/src/domain/metric_results/mod.rs
  • src/backend/services/analytics/src/domain/metric_results/validation.rs
  • src/backend/services/analytics/src/domain/metric_results/view.rs
  • src/backend/services/analytics/src/migration/m20260709_000001_metric_median_computation.rs
  • src/backend/services/analytics/src/migration/mod.rs
  • src/ingestion/dbt/macros/git_file_category.sql
  • src/ingestion/dbt/macros/metric_observation_measures.sql
  • src/ingestion/dbt/tests/git/assert_git_actor_emails_unique.sql
  • src/ingestion/dbt/tests/gold/assert_git_observations_dimension_values.sql
  • src/ingestion/dbt/tests/gold/assert_git_observations_entity_id_shape.sql
  • src/ingestion/dbt/tests/gold/assert_git_observations_nonnegative.sql
  • src/ingestion/gold/git_metric_observations.sql
  • src/ingestion/gold/schema.yml
  • src/ingestion/scripts/create-bronze-placeholders.sh
  • src/ingestion/silver/git/README.md
  • src/ingestion/silver/git/git_actor_emails.sql
  • src/ingestion/silver/git/schema.yml

Comment thread docs/domain/metrics/specs/DESIGN.md
Comment thread src/ingestion/gold/git_metric_observations.sql Outdated
Comment thread src/ingestion/gold/git_metric_observations.sql
Comment thread src/ingestion/scripts/create-bronze-placeholders.sh Outdated
Comment thread src/ingestion/silver/git/git_actor_emails.sql Outdated
aleksdotbar and others added 2 commits July 9, 2026 12:24
The name bridge (identity.git_actor_emails) was the tier-3 fallback
behind direct author email and the per-PR dominant-commit-email
crosswalk. Measured against real data, those two tiers resolve the vast
majority of PR authors on their own; the name bridge covered only a
small remainder and was the one tier that could emit a wrong-but-
plausible email from a shared name.

Remove it: PR authorship now resolves from the PR's own author_email or
the dominant email of its linked commits, and a PR that resolves to
neither is excluded (honest absence) rather than name-matched. This also
drops the identity_inputs cross-schema dependency, the sipHash tenant
join, and the fresh-cluster identity placeholders they required.

Deletes git_actor_emails.sql + its contract and grain test, the bridge
tier in the gold view, and the identity placeholder additions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
- Deterministic dedup: commits_source orders by the full commit key
  before LIMIT 1 BY, so the surviving copy of a forked commit (and the
  project_key/repo_slug the file-change join keys on) is stable across
  runs instead of arbitrary.

- Centralize the source-dimension display label in a git_source_label
  macro (mirroring git_file_category_label) instead of duplicating the
  vendor mapping inline in two CTEs; the header no longer claims the
  model carries no label mappings, and drops the stale 'identity bridge'
  mention left by the bridge removal.

- DESIGN 'new observation source' steps drop the removed ObservationSource
  enum / from_ref/table_ref mapping: adding a source is a dbt model plus a
  builtin registry seed whose source_ref is validated as an
  ObservationRelation and probed at runtime.

Skipped: the identity-bridge review comments (git_actor_emails and its
identity_inputs placeholder were already removed) and the window-alias
nitpick (max(uniqExact(...)) OVER is verified-working; referencing a
select alias inside a window is not reliably supported in ClickHouse).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
aleksdotbar and others added 4 commits July 9, 2026 18:02
Reconciles two overlapping reworks of the metric-results query path:

- #1701 batched period/peer into wide per-source queries (batch module,
  plan_queries, PeriodWideRow/PeerWideRow, item_value_expr).
- This branch added the median computation, the histogram view, and the
  ObservationRelation refactor (source_ref is validated data, not an enum).

Integration: median plugs into item_value_expr as one wide column
(quantileExactIfOrNull so an entity with no rows for the measure comes
back NULL, not zero) and into the per-view timeseries/breakdown arms;
histogram is a new UnbatchedView::Single alongside timeseries/breakdown.
#1701's new batch and compiler code is re-pointed from the removed
ObservationSource enum to observation_relation()/source_ref(), with the
period/peer group keys now String-keyed.

Verified: analytics build + 427 tests + clippy clean, full workspace
build, dbt parse; quantileExactIfOrNull confirmed valid on ClickHouse
and a real median metric resolves through the batched git period query.

Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Fold median into the mixed-computation batch test so an interleaved median column (the real git batch shape) is exercised by the placeholder-count assertion, not only single-computation batches.

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>
@aleksdotbar
aleksdotbar enabled auto-merge July 10, 2026 07:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/ingestion/dbt/macros/git_file_category.sql (1)

29-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor: package-lock\.json lacks end-anchor in config regex.

In the git_file_category config branch (line 14), package-lock\.json is the only pattern without a $ end-anchor, so a hypothetical package-lock.json.bak would classify as config. The sibling yarn.lock is covered by \.lock$ so it's fine, but for consistency consider anchoring: package-lock\\.json$.

This is a nitpick with negligible real-world impact — flagging only for regex consistency.

🤖 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/dbt/macros/git_file_category.sql` around lines 29 - 39, Anchor
the package-lock pattern in the git_file_category config branch by changing
package-lock\.json to package-lock\.json$, keeping the existing classification
behavior while preventing suffix matches such as package-lock.json.bak.
src/backend/services/analytics/src/domain/metric_results/batch.rs (1)

108-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add regression coverage for unbatched histogram planning.

Add a test asserting that multiple histogram views each produce a distinct PlannedQuery::Single with UnbatchedView::Histogram. This protects the distinct execution contract established in validation.rs:41-54 and consumed by builder.rs:158-215.

🤖 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/batch.rs` around
lines 108 - 115, Add regression coverage for the unbatched histogram branch in
the metric planning test suite: construct a request containing multiple
histogram views, invoke the relevant planning function, and assert each produces
a distinct PlannedQuery::Single with the correct metric_index, view_index, and
UnbatchedView::Histogram.
🤖 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.

Nitpick comments:
In `@src/backend/services/analytics/src/domain/metric_results/batch.rs`:
- Around line 108-115: Add regression coverage for the unbatched histogram
branch in the metric planning test suite: construct a request containing
multiple histogram views, invoke the relevant planning function, and assert each
produces a distinct PlannedQuery::Single with the correct metric_index,
view_index, and UnbatchedView::Histogram.

In `@src/ingestion/dbt/macros/git_file_category.sql`:
- Around line 29-39: Anchor the package-lock pattern in the git_file_category
config branch by changing package-lock\.json to package-lock\.json$, keeping the
existing classification behavior while preventing suffix matches such as
package-lock.json.bak.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 366d1a9d-9e22-4c0d-8b11-867ebfd10c6d

📥 Commits

Reviewing files that changed from the base of the PR and between ed3489d and 83d52bd.

📒 Files selected for processing (11)
  • docs/domain/metrics/specs/DESIGN.md
  • src/backend/services/analytics/src/api/metric_results.rs
  • src/backend/services/analytics/src/domain/metric_definitions/definition.rs
  • src/backend/services/analytics/src/domain/metric_results/batch.rs
  • src/backend/services/analytics/src/domain/metric_results/builder.rs
  • src/backend/services/analytics/src/domain/metric_results/compiler.rs
  • src/backend/services/analytics/src/domain/metric_results/mod.rs
  • src/backend/services/analytics/src/domain/metric_results/validation.rs
  • src/ingestion/dbt/macros/git_file_category.sql
  • src/ingestion/gold/git_metric_observations.sql
  • src/ingestion/scripts/create-bronze-placeholders.sh
✅ Files skipped from review due to trivial changes (1)
  • src/backend/services/analytics/src/domain/metric_results/mod.rs
🚧 Files skipped from review as they are similar to previous changes (8)
  • src/backend/services/analytics/src/api/metric_results.rs
  • src/ingestion/scripts/create-bronze-placeholders.sh
  • docs/domain/metrics/specs/DESIGN.md
  • src/backend/services/analytics/src/domain/metric_definitions/definition.rs
  • src/ingestion/gold/git_metric_observations.sql
  • src/backend/services/analytics/src/domain/metric_results/validation.rs
  • src/backend/services/analytics/src/domain/metric_results/builder.rs
  • src/backend/services/analytics/src/domain/metric_results/compiler.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants