From 5c5b4797ac4479ef66a9bda883df4d84b2e43caf Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 8 Sep 2026 17:28:19 +0200 Subject: [PATCH 1/2] Measure lead time per commit, as DORA defines it Lead time was the newest commit in each release, which answers "how stale was the freshest change" rather than "how long from commit to running". On the production export the difference is sevenfold: 26.7 h as a proxy against a real 193.6 h median, and the proxy is the flattering one. Three views do it properly. commit_sightings expands the commits array that push payloads already carry - id, author timestamp, parents, author type - into one row per commit on master. deploy_commit_ranges says which commit range a deploy shipped, today from the release-note bump links; that is the one view image_ref will replace, with "between the previous and current deploy of this app". lead_time_changes maps each commit to the first deploy per environment that carried it, so a release rolling out to four Apps counts each change once. The SQL lives in riptide_collector.views so the migration and the tests run the same text - an untestable metric definition drifts. Merge commits and service-account commits are classified, not dropped, and the documented default excludes both: they are artifacts of shipping, not changes that shipped. Bots get their own line rather than being blended, since they authored 87 percent of commits reaching production. Measured against the export: prod human p50 193.6 h / p90 505.6 h over 984 changes, intg p50 4.0 h. The gap between the two is the finding - delivery is fast, the wait sits in front of production. --- AGENTS.md | 1 + README.md | 4 +- docs/dora-lead-time.md | 88 +++++ migrations/versions/0004_lead_time_views.py | 45 +++ src/riptide_collector/views.py | 134 ++++++++ tests/test_lead_time_views.py | 340 ++++++++++++++++++++ 6 files changed, 611 insertions(+), 1 deletion(-) create mode 100644 docs/dora-lead-time.md create mode 100644 migrations/versions/0004_lead_time_views.py create mode 100644 src/riptide_collector/views.py create mode 100644 tests/test_lead_time_views.py diff --git a/AGENTS.md b/AGENTS.md index 6c9acb9..6d75604 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,7 @@ podman-compose up # Postgres + migrations + app on :8000 - **Metrics computed on read.** No aggregation tables, no rollup jobs in v1. Schema additions preserve raw events. - **Correlation, in priority order.** Bitbucket↔Pipeline: `commit_sha` (App-repo SHA both sides, deterministic). Argo CD: the **full image reference** — senders report `pipeline_events.image_ref` (`registry/path:tag`), Argo stores the same strings in `payload->'images'`. `argocd_events.revision` is the GitOps-repo SHA (four Apps of one service share one) and matches neither other source. Image **tags are not SHAs** — measured: 0 of 4 936 refs, all semver; never parse a SHA out of a tag. Pre-`image_ref` rows: read-time fallback in `docs/correlating-deploys-to-commits.md`. Never `service_id` or name mappings. - **`repo:refs_changed` is ref movement, not developer activity.** Measured: of 16 295 master-ref events ~15 000 were release tooling (maven/gradle release plugins, component-version job, Renovate); the 1 210 human-authored ones were merge commits already counted as `pr:merged`. Read activity and `change_type` off PR events — a `master` push has no branch prefix, so change mix over all events reads 83 % `other`. Never infer intent from an event-type name; check `author` and the commit message. +- **Lead time is per commit, against the first deploy that carried it** — the `lead_time_changes` view (over `commit_sightings` + `deploy_commit_ranges`). Exclude merge commits and service-account commits; report bots as their own line, never blended (87 % of commits reaching prod were Renovate's). Never quote the newest-commit-per-release shortcut as lead time: measured 26.7 h against a real 193.6 h. - **`change_type` on Bitbucket events only.** Don't denormalise onto pipeline / Argo rows; join at read time. - **Automation detection is config-last.** Order: configured `automation` authors (matched against login *and* display name, case-insensitive) → acting user's `type == "SERVICE"` from the payload → `*-bot` name shape. Senders also declare themselves (`reviewer_handle` / `actor_handle` + account kind, read-time filter). Only accounts nobody reports get a config entry. `automation` is org-wide, at the config root. - **CI events are source-tagged, not source-routed.** Every CI lands in `pipeline_events` via `POST /webhooks/pipeline`, told apart by `source`. No per-CI tables or endpoints. Dedup key `source#pipeline_name#run_id#phase`. diff --git a/README.md b/README.md index 67ea6e1..941ea4c 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,7 @@ the data captured in v1. | Metric | How it's computed | |---|---| | **Deployment frequency** | `COUNT(DISTINCT revision)` of `argocd_events` per `team` / time window where `operation_phase = 'Succeeded' AND environment = 'prod'`. Count revisions, not rows: one release reconciles every Argo App that shares the GitOps revision, so `COUNT(*)` per `app_name` overcounts releases. Group by `app_name` for the per-App drill-down. | -| **Lead time for changes** | Deploy → build → commit, joined on the image reference: `argocd_events.payload->'images'` contains the full image refs Argo rendered, `pipeline_events.image_ref` is what the build published. From the pipeline row, `commit_sha` gives the App-repo commit; its first sighting in `bitbucket_events` starts the clock, the `argocd_events.occurred_at` of the prod deploy ends it. Stratify by `bitbucket_events.change_type` (feature / hotfix / bugfix / …) to separate hotfix from feature lead time. `argocd_events.revision` is the GitOps-repo SHA and does **not** join to `commit_sha` — see [Correlating deploys back to commits](docs/correlating-deploys-to-commits.md), which also documents the read-time fallback for events collected before senders reported `image_ref`. | +| **Lead time for changes** | Per commit, not per release: `SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY lead_time) FROM lead_time_changes WHERE environment='prod' AND NOT is_merge AND NOT author_is_service_account`. The `lead_time_changes` view maps every commit that shipped to the **first** deploy that carried it, so a release rolled out to four Apps counts each change once. Exclude merge commits and release tooling, and report bots on their own line — see [Lead time for changes (DORA)](docs/dora-lead-time.md) for why, and for the coverage limits. Stratify by `bitbucket_events.change_type` for hotfix vs feature. Do **not** substitute the newest commit per release: it answers "how stale was the freshest change" and reads ~7× too fast. | | **PR cycle time** | `pullrequest:fulfilled.occurred_at − pullrequest:created.occurred_at` per PR id. | | **Time to first review** *(DX Core 4 "code review pickup time")* | Two-part computation per PR — see the SQL block below the table. Clock-start = `COALESCE(pr:ready_for_review, pr:opened)`: PRs opened ready start at `pr:opened`; PRs opened as drafts start at the synthetic `pr:ready_for_review` (emitted by the parser when a `pr:modified` payload carries `previousDraft=true, draft=false`). Engagement = first reviewer touch (`pr:comment:added`, `pr:reviewer:approved`, `pr:reviewer:unapproved`, `pr:reviewer:needs_work`, `pr:reviewer:updated`) where `author != pr_opener AND NOT is_automated AND occurred_at >= clock-start`. The five-event reviewer union covers every touch Bitbucket DC emits (silent approvals, retracted approvals, "needs work" flips, bare reviewer-status changes); the `occurred_at >= clock-start` guard drops early-feedback comments solicited during the draft phase, which would otherwise produce negative pickup times. `NOT is_automated` strips bot comments (noergler / Renovate / etc.) — every review-time bot must be recognisable, otherwise its instant comment drives the metric toward zero. Detection matches the `automation` config block against **both** the login handle (`author`) and the display name (`author_display_name`), because a bot is often provisioned as an ordinary user account whose login says nothing. `is_automated` is decided at ingest, so rows written before a bot was recognised stay marked human: the `non_human_identities` CTE in the query below filters those at read time from the accounts senders declare (`reviewer_handle` + `reviewer_account_kind` on noergler rollups, `actor_handle` + `actor_account_kind` on pipeline events) — riptide stores the declaration, it carries no account names of its own. | | **Build success rate** | `pipeline_events` with `phase = 'COMPLETED'` grouped by `status`. Slice by `source` to compare Jenkins vs Tekton, by `pipeline_name` / `team` for ownership. | @@ -312,4 +312,6 @@ See [`docs/`](docs/) for setup and onboarding guides: - [Setup: ArgoCD notification](docs/setup-argocd-notification.md) - [Setup: Noergler notification](docs/setup-noergler-notification.md) - [Onboarding a team](docs/onboarding-a-team.md) +- [Lead time for changes (DORA)](docs/dora-lead-time.md) +- [Correlating deploys back to commits](docs/correlating-deploys-to-commits.md) - [OpenShift manifests](openshift/README.md) diff --git a/docs/dora-lead-time.md b/docs/dora-lead-time.md new file mode 100644 index 0000000..6633a27 --- /dev/null +++ b/docs/dora-lead-time.md @@ -0,0 +1,88 @@ +# Lead time for changes (DORA), per commit + +**How long from a developer committing to that change running in production.** +One row per change, not per release: DORA's second key metric. + +Riptide ships three views for it. Query `lead_time_changes`; the two below it +exist so the fragile part stays swappable. + +| View | One row per | Purpose | +|---|---|---| +| `commit_sightings` | commit seen on a master push | expands `payload->'commits'`, with timestamps and classification | +| `deploy_commit_ranges` | (deploy, app repo) | which commit range a deploy shipped | +| `lead_time_changes` | (environment, commit) | the change and the **first** deploy that carried it | + +## The metric + +```sql +-- Lead time to production, human-authored changes, last 90 days +SELECT + percentile_cont(0.5) WITHIN GROUP (ORDER BY lead_time) AS p50, + percentile_cont(0.9) WITHIN GROUP (ORDER BY lead_time) AS p90, + count(*) AS changes +FROM lead_time_changes +WHERE environment = 'prod' + AND NOT is_merge + AND NOT author_is_service_account + AND author NOT IN (SELECT unnest(:automation_handles)) + AND first_deployed_at > now() - interval '90 days'; +``` + +Group by `team`, `repo_full_name`, or `date_trunc('week', first_deployed_at)` +for the breakdowns. Swap `environment` for the stage you treat as production. + +## What counts as a change + +The views classify; the query decides. The defaults that make the number mean +what people think it means: + +- **Exclude merge commits** (`NOT is_merge`). A squash-and-merge lands the change + once; counting the merge commit too double-counts it. +- **Exclude release tooling** (`NOT author_is_service_account`, plus your CI + account if it is not flagged `SERVICE` by the git host). `[maven-release-plugin] + prepare for next development iteration`, `[gradle-release] …` and + component-version commits are created *by* the release, so their lead time is + near zero and they drag the median down. +- **Report bots separately, never blended.** Dependency updates are real changes + that ship, but in one measured dataset Renovate authored 87 % of all commits + reaching production. Blending makes the median describe the bot's cadence + (p50 209.3 h) rather than the team's (p50 193.6 h). Two lines, always. + +## Reading it + +Lower is better, and the split between environments is where the signal is. In +the dataset this was built on, human changes reached **intg in 4.0 h (p50)** but +**production in 193.6 h (p50), p90 505.6 h** — the delivery pipeline is fast and +the wait is entirely in front of production, in release scheduling. A single +blended number would have hidden that. + +Beware the tempting shortcut this replaces: taking the *newest* commit in each +release and calling that lead time. It answers "how stale was the freshest +change" and reads 26.7 h on the same data — 7× too flattering. + +## Limits, so nobody over-reads the number + +- **`authored_at` is the commit's own timestamp**, which a rebase rewrites. + That is DORA's "code committed"; `committed_at` sits next to it for comparison. +- **Coverage is bounded by range resolution.** A deploy whose commit range cannot + be resolved contributes nothing — it is absent, never counted as fast. Check it: + + ```sql + SELECT count(*) AS deploys, + count(*) FILTER (WHERE r.deployed_at IS NULL) AS unresolved + FROM argocd_events a + LEFT JOIN deploy_commit_ranges r + ON r.app_name = a.app_name AND r.deployed_at = a.occurred_at + WHERE a.operation_phase = 'Succeeded'; + ``` + +- **Bitbucket caps `commits[]` at 5 per push.** Measured, that bites 0.6 % of + master pushes; it drops changes rather than mis-timing the ones it keeps. +- **Ranges come from release notes today.** `deploy_commit_ranges` reads the + compare links a release-note generator writes into the GitOps commit, so a + release landed as a direct version bump is invisible to it. That is the view to + replace once CI senders report `pipeline_events.image_ref`: the range then + becomes "between the previous and the current successful deploy of this app", + which needs no release notes at all. See + [Correlating deploys back to commits](correlating-deploys-to-commits.md). +- **Ingestion is forward-only.** The metric covers what riptide has seen. diff --git a/migrations/versions/0004_lead_time_views.py b/migrations/versions/0004_lead_time_views.py new file mode 100644 index 0000000..560f33c --- /dev/null +++ b/migrations/versions/0004_lead_time_views.py @@ -0,0 +1,45 @@ +"""views for per-commit DORA lead time + +Lead time was a proxy: one App-repo commit per release (the newest), which +answers "how stale was the freshest change" rather than DORA's "how long from +commit to running in production". On real data the difference is 7x — prod p50 +26.7 h as a proxy against 193.6 h per commit — and the proxy is the flattering +one, so it is replaced rather than kept alongside. + +The view SQL lives in `riptide_collector.views` so this migration and the tests +execute the same definition. The index exists because the range lookups are per +repo and per time window: without it the metric query does not finish on a +table of any size. + +Revision ID: 0004 +Revises: 0003 +Create Date: 2026-09-08 + +""" + +from collections.abc import Sequence + +from alembic import op + +from riptide_collector.views import CREATE_VIEWS, DROP_VIEWS + +revision: str = "0004" +down_revision: str | None = "0003" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + op.create_index( + "ix_bitbucket_events_repo_branch_occurred", + "bitbucket_events", + ["repo_full_name", "branch_name", "occurred_at"], + ) + for statement in CREATE_VIEWS: + op.execute(statement) + + +def downgrade() -> None: + for statement in DROP_VIEWS: + op.execute(statement) + op.drop_index("ix_bitbucket_events_repo_branch_occurred", table_name="bitbucket_events") diff --git a/src/riptide_collector/views.py b/src/riptide_collector/views.py new file mode 100644 index 0000000..61ae2ec --- /dev/null +++ b/src/riptide_collector/views.py @@ -0,0 +1,134 @@ +"""Read-time views for metrics that need more than a single table scan. + +The definitions live here rather than inline in the migration so that the +migration and the tests execute the same SQL — a view is a metric definition, +and a definition nobody can test drifts. + +Layered on purpose: + +- `commit_sightings` expands `payload->'commits'` from master pushes into one + row per commit, with the timestamps and the classification the metrics filter + on (merge commit, service account, author). +- `deploy_commit_ranges` says which commit range a deploy shipped. This is the + fragile one: today it reads the release-note bump links out of the GitOps + commit. When senders report `pipeline_events.image_ref` it becomes "between + the previous and the current successful deploy of this app" and nothing else + changes. +- `lead_time_changes` maps every commit to the FIRST successful deploy per + environment whose range contained it, so a change rolled out to four Apps of + one service counts once. + +Plain views, not materialized: nothing to refresh, so the no-rollup-jobs +invariant holds. +""" + +# Master pushes only: a change enters the release stream when it lands on the +# integration branch, and feature-branch pushes would count it twice. Bitbucket +# caps `commits[]` at 5 per push — measured, that bites 0.6 % of master pushes, +# and it drops changes rather than mis-timing the ones it keeps. +COMMIT_SIGHTINGS = """ +CREATE VIEW commit_sightings AS +SELECT + b.repo_full_name, + b.team, + c ->> 'id' AS commit_sha, + to_timestamp((c ->> 'authorTimestamp')::bigint / 1000) AS authored_at, + to_timestamp((c ->> 'committerTimestamp')::bigint / 1000) AS committed_at, + b.occurred_at AS seen_at, + c -> 'author' ->> 'name' AS author, + c -> 'author' ->> 'displayName' AS author_display_name, + -- The git host states this itself; a SERVICE account is release tooling, + -- and its commits are created by the release rather than shipped by it. + upper(coalesce(c -> 'author' ->> 'type', 'NORMAL')) = 'SERVICE' + AS author_is_service_account, + coalesce(jsonb_array_length(c -> 'parents'), 0) > 1 AS is_merge, + c ->> 'message' AS message +FROM bitbucket_events b +CROSS JOIN LATERAL jsonb_array_elements(b.payload -> 'commits') AS c +WHERE b.event_type = 'repo:refs_changed' + AND b.branch_name = 'master' +""" + +# The release commit in the GitOps repo lists every component it bumped as a +# compare link carrying the previous and the new App-repo SHA; those two bound +# the range the deploy shipped. The LIKE prefilter keeps the regex off every +# push payload in the table. +DEPLOY_COMMIT_RANGES = r""" +CREATE VIEW deploy_commit_ranges AS +WITH release_commit AS ( + SELECT DISTINCT + a.app_name, + a.environment, + a.team, + a.occurred_at AS deployed_at, + b.payload::text AS body + FROM argocd_events a + JOIN bitbucket_events b ON b.commit_sha = a.revision + WHERE a.operation_phase = 'Succeeded' + AND b.payload::text LIKE '%compare/diff?targetBranch=%' +), +bump AS ( + SELECT + app_name, environment, team, deployed_at, + regexp_matches( + body, + 'repos/([a-z0-9-]+)/compare/diff\?targetBranch=([0-9a-f]{40})&sourceBranch=([0-9a-f]{40})', + 'g' + ) AS m + FROM release_commit +) +SELECT + bump.app_name, + bump.environment, + bump.team, + bump.deployed_at, + prev.repo_full_name, + bump.m[2] AS range_start_sha, + bump.m[3] AS range_end_sha, + prev.seen_at AS range_start_at, + curr.seen_at AS range_end_at +FROM bump +JOIN commit_sightings prev + ON prev.commit_sha = bump.m[2] + AND prev.repo_full_name LIKE '%/' || bump.m[1] +JOIN commit_sightings curr + ON curr.commit_sha = bump.m[3] + AND curr.repo_full_name = prev.repo_full_name +""" + +# First deploy only: a change reaches production once, even though the release +# rolls out to every App of the service. +LEAD_TIME_CHANGES = """ +CREATE VIEW lead_time_changes AS +SELECT + r.environment, + s.repo_full_name, + s.team, + s.commit_sha, + s.authored_at, + s.committed_at, + s.author, + s.author_display_name, + s.author_is_service_account, + s.is_merge, + min(r.deployed_at) AS first_deployed_at, + min(r.deployed_at) - s.authored_at AS lead_time +FROM deploy_commit_ranges r +JOIN commit_sightings s + ON s.repo_full_name = r.repo_full_name + AND s.seen_at > r.range_start_at + AND s.seen_at <= r.range_end_at +GROUP BY + r.environment, s.repo_full_name, s.team, s.commit_sha, s.authored_at, + s.committed_at, s.author, s.author_display_name, + s.author_is_service_account, s.is_merge +HAVING min(r.deployed_at) > s.authored_at +""" + +# Dependency order: each view builds on the one before it. +CREATE_VIEWS = (COMMIT_SIGHTINGS, DEPLOY_COMMIT_RANGES, LEAD_TIME_CHANGES) +DROP_VIEWS = ( + "DROP VIEW IF EXISTS lead_time_changes", + "DROP VIEW IF EXISTS deploy_commit_ranges", + "DROP VIEW IF EXISTS commit_sightings", +) diff --git a/tests/test_lead_time_views.py b/tests/test_lead_time_views.py new file mode 100644 index 0000000..f961912 --- /dev/null +++ b/tests/test_lead_time_views.py @@ -0,0 +1,340 @@ +"""Tests for the DORA lead-time views (`riptide_collector.views`). + +The views are the metric definition, so they are exercised as SQL against a +real Postgres with hand-built events: one app repo whose commits land on +master, a GitOps release commit naming the range it shipped, and Argo CD +deploys of that revision. +""" + +from __future__ import annotations + +import json +from datetime import UTC, datetime, timedelta +from typing import Any + +import pytest_asyncio +from sqlalchemy import text +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from riptide_collector.views import CREATE_VIEWS, DROP_VIEWS + +APP_REPO = "acme/payments-api" +GITOPS_REPO = "acme/payments-infra" +BASE = datetime(2026, 4, 1, 8, 0, tzinfo=UTC) + +# The range endpoints: the App-repo commit the previous release shipped, and +# the one this release ships. +SHA_PREV = "a" * 40 +SHA_HEAD = "d" * 40 + + +def _commit( + sha: str, + *, + at: datetime, + author: str = "alice", + author_type: str = "NORMAL", + parents: int = 1, + message: str = "change", +) -> dict[str, Any]: + epoch_ms = int(at.timestamp() * 1000) + return { + "id": sha, + "message": message, + "authorTimestamp": epoch_ms, + "committerTimestamp": epoch_ms, + "author": {"name": author, "displayName": author, "type": author_type}, + "parents": [{"id": "0" * 40}] * parents, + } + + +async def _push( + session: AsyncSession, + *, + delivery_id: str, + repo: str, + at: datetime, + commits: list[dict[str, Any]], + branch: str = "master", +) -> None: + payload = {"eventKey": "repo:refs_changed", "commits": commits} + await session.execute( + text( + "INSERT INTO bitbucket_events " + "(delivery_id, event_type, repo_full_name, branch_name, commit_sha, " + " occurred_at, team, jira_keys, payload) " + "VALUES (:d, 'repo:refs_changed', :repo, :branch, :sha, :at, 'checkout', " + " '{}', CAST(:payload AS jsonb))" + ), + { + "d": delivery_id, + "repo": repo, + "branch": branch, + "sha": commits[-1]["id"], + "at": at, + "payload": json.dumps(payload), + }, + ) + + +async def _deploy( + session: AsyncSession, + *, + delivery_id: str, + app: str, + environment: str, + revision: str, + at: datetime, +) -> None: + await session.execute( + text( + "INSERT INTO argocd_events " + "(delivery_id, app_name, revision, operation_phase, environment, " + " occurred_at, team, payload) " + "VALUES (:d, :app, :rev, 'Succeeded', :env, :at, 'checkout', '{}')" + ), + {"d": delivery_id, "app": app, "rev": revision, "env": environment, "at": at}, + ) + + +def _release_note(old: str, new: str) -> str: + # The shape a release-note generator writes into the GitOps commit message. + return ( + "Payments PROD 2.0.41\n\n" + "[__payments-api 2.0.37 → 2.0.41__]" + f"(https://git.example.com/projects/acme/repos/payments-api/compare/diff" + f"?targetBranch={old}&sourceBranch={new})" + ) + + +@pytest_asyncio.fixture +async def seeded( + session_factory: async_sessionmaker[AsyncSession], +) -> async_sessionmaker[AsyncSession]: + """One release shipping four commits, deployed to intg then prod.""" + async with session_factory() as session: + # Previous release's head — the range starts after this one. + await _push( + session, + delivery_id="p0", + repo=APP_REPO, + at=BASE, + commits=[_commit(SHA_PREV, at=BASE, message="previous release head")], + ) + # Three changes land on master, each committed two hours apart. + await _push( + session, + delivery_id="p1", + repo=APP_REPO, + at=BASE + timedelta(hours=2), + commits=[_commit("b" * 40, at=BASE + timedelta(hours=2))], + ) + await _push( + session, + delivery_id="p2", + repo=APP_REPO, + at=BASE + timedelta(hours=4), + commits=[ + _commit( + "c" * 40, + at=BASE + timedelta(hours=4), + parents=2, + message="Pull request #7: merge", + ), + ], + ) + await _push( + session, + delivery_id="p3", + repo=APP_REPO, + at=BASE + timedelta(hours=6), + commits=[ + _commit( + SHA_HEAD, + at=BASE + timedelta(hours=6), + author="ci-service", + author_type="SERVICE", + message="[maven-release-plugin] prepare release payments-api-2.0.41", + ), + ], + ) + # A feature-branch push must not enter the metric. + await _push( + session, + delivery_id="p4", + repo=APP_REPO, + at=BASE + timedelta(hours=5), + branch="feature/x", + commits=[_commit("e" * 40, at=BASE + timedelta(hours=5))], + ) + # The GitOps release commit naming the range, then its deploys. + await _push( + session, + delivery_id="g1", + repo=GITOPS_REPO, + at=BASE + timedelta(hours=7), + commits=[ + _commit( + "f" * 40, + at=BASE + timedelta(hours=7), + message=_release_note(SHA_PREV, SHA_HEAD), + ), + ], + ) + await _deploy( + session, + delivery_id="d-intg", + app="payments-intg", + environment="intg", + revision="f" * 40, + at=BASE + timedelta(hours=8), + ) + # Same revision, two prod Apps: one change must not count twice. + await _deploy( + session, + delivery_id="d-prod-1", + app="payments-intranet-prod", + environment="prod", + revision="f" * 40, + at=BASE + timedelta(hours=10), + ) + await _deploy( + session, + delivery_id="d-prod-2", + app="payments-extranet-prod", + environment="prod", + revision="f" * 40, + at=BASE + timedelta(hours=12), + ) + await session.commit() + + async with session_factory() as session: + for statement in DROP_VIEWS: + await session.execute(text(statement)) + for statement in CREATE_VIEWS: + await session.execute(text(statement)) + await session.commit() + + return session_factory + + +class TestCommitSightings: + async def test_only_master_commits_are_sighted(self, seeded: Any) -> None: + async with seeded() as session: + rows = ( + ( + await session.execute( + text("SELECT commit_sha FROM commit_sightings WHERE repo_full_name = :r"), + {"r": APP_REPO}, + ) + ) + .scalars() + .all() + ) + + # The feature-branch commit is absent: a change enters the release + # stream when it lands on master, and counting both double-counts it. + assert "e" * 40 not in rows + assert {SHA_PREV, "b" * 40, "c" * 40, SHA_HEAD} == set(rows) + + async def test_classification_columns(self, seeded: Any) -> None: + async with seeded() as session: + rows = dict( + ( + await session.execute( + text( + "SELECT commit_sha, (is_merge, author_is_service_account)::text " + "FROM commit_sightings WHERE repo_full_name = :r" + ), + {"r": APP_REPO}, + ) + ).all() + ) + + assert rows["c" * 40] == "(t,f)" # merge commit + assert rows[SHA_HEAD] == "(f,t)" # release tooling, SERVICE account + assert rows["b" * 40] == "(f,f)" # an ordinary change + + +class TestLeadTimeChanges: + async def test_lead_time_measured_from_commit_to_first_deploy(self, seeded: Any) -> None: + async with seeded() as session: + row = ( + await session.execute( + text( + "SELECT extract(epoch FROM lead_time)/3600 FROM lead_time_changes " + "WHERE environment = 'prod' AND commit_sha = :sha" + ), + {"sha": "b" * 40}, + ) + ).scalar_one() + + # Committed at BASE+2h, first prod deploy at BASE+10h. + assert row == 8 + + async def test_second_deploy_of_the_same_release_does_not_recount(self, seeded: Any) -> None: + async with seeded() as session: + count = ( + await session.execute( + text( + "SELECT count(*) FROM lead_time_changes " + "WHERE environment = 'prod' AND commit_sha = :sha" + ), + {"sha": "b" * 40}, + ) + ).scalar_one() + + # Two prod Apps shipped it; it reached production once. + assert count == 1 + + async def test_range_excludes_the_previous_release_head(self, seeded: Any) -> None: + async with seeded() as session: + shas = ( + ( + await session.execute( + text("SELECT commit_sha FROM lead_time_changes WHERE environment = 'prod'") + ) + ) + .scalars() + .all() + ) + + # The range is (previous head, this head]: the old head shipped last time. + assert SHA_PREV not in shas + assert set(shas) == {"b" * 40, "c" * 40, SHA_HEAD} + + async def test_environments_are_measured_separately(self, seeded: Any) -> None: + async with seeded() as session: + rows = dict( + ( + await session.execute( + text( + "SELECT environment, extract(epoch FROM lead_time)/3600 " + "FROM lead_time_changes WHERE commit_sha = :sha" + ), + {"sha": "b" * 40}, + ) + ).all() + ) + + assert rows == {"intg": 6, "prod": 8} + + async def test_default_metric_filter_keeps_only_real_changes(self, seeded: Any) -> None: + async with seeded() as session: + shas = ( + ( + await session.execute( + text( + "SELECT commit_sha FROM lead_time_changes " + "WHERE environment = 'prod' " + " AND NOT is_merge AND NOT author_is_service_account" + ) + ) + ) + .scalars() + .all() + ) + + # The merge commit and the release-plugin commit are artifacts of + # shipping, not changes that were shipped. + assert shas == ["b" * 40] From 58ed8ff5b4c7de6fe0aa462eeb0fa0ec1036452c Mon Sep 17 00:00:00 2001 From: trick77 Date: Tue, 8 Sep 2026 17:42:17 +0200 Subject: [PATCH 2/2] Address review findings on the lead-time views - Join the App repo by exact slug instead of LIKE, and widen the slug pattern to allow '_' and '.', which are legal and would otherwise yield no ranges at all - with LIKE, '_' would also have matched any character. - Declare the new index on the model too, so autogenerate does not propose dropping it. - Doc query: match bots on login and display name, the same rule the rest of riptide uses, and with NOT EXISTS so an unresolved author is kept rather than dropped by NULL propagation. - Coverage query counted bumps, not deploys, which understated the unresolved share. Counts distinct deploys now: 365 of 1007 unresolved on the export. - Write down the two boundary assumptions rather than leaving them implicit: boundary SHAs and the GitOps release commit are push tips, and a missing boundary costs a whole release window, not one change, because the next range starts where this one ended. - Test a two-commit push, which nothing covered: both commits are attributed to the same release and each keeps its own commit timestamp. - README no longer suggests stratifying by change_type, which these commits do not carry. --- README.md | 2 +- docs/dora-lead-time.md | 32 ++++++++++++++++++++---- src/riptide_collector/models.py | 8 ++++++ src/riptide_collector/views.py | 26 ++++++++++++++++++-- tests/test_lead_time_views.py | 43 ++++++++++++++++++++++++++------- 5 files changed, 94 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 941ea4c..d644763 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,7 @@ the data captured in v1. | Metric | How it's computed | |---|---| | **Deployment frequency** | `COUNT(DISTINCT revision)` of `argocd_events` per `team` / time window where `operation_phase = 'Succeeded' AND environment = 'prod'`. Count revisions, not rows: one release reconciles every Argo App that shares the GitOps revision, so `COUNT(*)` per `app_name` overcounts releases. Group by `app_name` for the per-App drill-down. | -| **Lead time for changes** | Per commit, not per release: `SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY lead_time) FROM lead_time_changes WHERE environment='prod' AND NOT is_merge AND NOT author_is_service_account`. The `lead_time_changes` view maps every commit that shipped to the **first** deploy that carried it, so a release rolled out to four Apps counts each change once. Exclude merge commits and release tooling, and report bots on their own line — see [Lead time for changes (DORA)](docs/dora-lead-time.md) for why, and for the coverage limits. Stratify by `bitbucket_events.change_type` for hotfix vs feature. Do **not** substitute the newest commit per release: it answers "how stale was the freshest change" and reads ~7× too fast. | +| **Lead time for changes** | Per commit, not per release: `SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY lead_time) FROM lead_time_changes WHERE environment='prod' AND NOT is_merge AND NOT author_is_service_account`. The `lead_time_changes` view maps every commit that shipped to the **first** deploy that carried it, so a release rolled out to four Apps counts each change once. Exclude merge commits and release tooling, and report bots on their own line — see [Lead time for changes (DORA)](docs/dora-lead-time.md) for why, and for the coverage limits. Group by `team` or `repo_full_name`; `change_type` is not available here, since these commits come from master pushes, which carry no branch prefix. Do **not** substitute the newest commit per release: it answers "how stale was the freshest change" and reads ~7× too fast. | | **PR cycle time** | `pullrequest:fulfilled.occurred_at − pullrequest:created.occurred_at` per PR id. | | **Time to first review** *(DX Core 4 "code review pickup time")* | Two-part computation per PR — see the SQL block below the table. Clock-start = `COALESCE(pr:ready_for_review, pr:opened)`: PRs opened ready start at `pr:opened`; PRs opened as drafts start at the synthetic `pr:ready_for_review` (emitted by the parser when a `pr:modified` payload carries `previousDraft=true, draft=false`). Engagement = first reviewer touch (`pr:comment:added`, `pr:reviewer:approved`, `pr:reviewer:unapproved`, `pr:reviewer:needs_work`, `pr:reviewer:updated`) where `author != pr_opener AND NOT is_automated AND occurred_at >= clock-start`. The five-event reviewer union covers every touch Bitbucket DC emits (silent approvals, retracted approvals, "needs work" flips, bare reviewer-status changes); the `occurred_at >= clock-start` guard drops early-feedback comments solicited during the draft phase, which would otherwise produce negative pickup times. `NOT is_automated` strips bot comments (noergler / Renovate / etc.) — every review-time bot must be recognisable, otherwise its instant comment drives the metric toward zero. Detection matches the `automation` config block against **both** the login handle (`author`) and the display name (`author_display_name`), because a bot is often provisioned as an ordinary user account whose login says nothing. `is_automated` is decided at ingest, so rows written before a bot was recognised stay marked human: the `non_human_identities` CTE in the query below filters those at read time from the accounts senders declare (`reviewer_handle` + `reviewer_account_kind` on noergler rollups, `actor_handle` + `actor_account_kind` on pipeline events) — riptide stores the declaration, it carries no account names of its own. | | **Build success rate** | `pipeline_events` with `phase = 'COMPLETED'` grouped by `status`. Slice by `source` to compare Jenkins vs Tekton, by `pipeline_name` / `team` for ownership. | diff --git a/docs/dora-lead-time.md b/docs/dora-lead-time.md index 6633a27..f701c68 100644 --- a/docs/dora-lead-time.md +++ b/docs/dora-lead-time.md @@ -24,7 +24,14 @@ FROM lead_time_changes WHERE environment = 'prod' AND NOT is_merge AND NOT author_is_service_account - AND author NOT IN (SELECT unnest(:automation_handles)) + -- Match the login AND the display name, the same rule the rest of riptide + -- uses: a bot is often provisioned with a nondescript login. NOT EXISTS + -- rather than NOT IN, so a commit with an unresolved author is kept rather + -- than silently dropped by NULL propagation. + AND NOT EXISTS ( + SELECT 1 FROM unnest(:automation_handles) AS h(handle) + WHERE lower(h.handle) IN (lower(author), lower(author_display_name)) + ) AND first_deployed_at > now() - interval '90 days'; ``` @@ -65,19 +72,34 @@ change" and reads 26.7 h on the same data — 7× too flattering. - **`authored_at` is the commit's own timestamp**, which a rebase rewrites. That is DORA's "code committed"; `committed_at` sits next to it for comparison. - **Coverage is bounded by range resolution.** A deploy whose commit range cannot - be resolved contributes nothing — it is absent, never counted as fast. Check it: + be resolved contributes nothing — it is absent, never counted as fast. Count + deploys, not bumps: one deploy fans out to a row per bumped component. ```sql - SELECT count(*) AS deploys, - count(*) FILTER (WHERE r.deployed_at IS NULL) AS unresolved + SELECT count(DISTINCT a.id) AS deploys, + count(DISTINCT a.id) FILTER (WHERE r.deployed_at IS NULL) AS unresolved FROM argocd_events a LEFT JOIN deploy_commit_ranges r ON r.app_name = a.app_name AND r.deployed_at = a.occurred_at WHERE a.operation_phase = 'Succeeded'; ``` +- **A missing boundary commit costs a whole release, not one change.** The range + needs both endpoint SHAs, so if either is absent — the `commits[]` cap below, + or an ingest gap — every change in that window disappears. It does not recover + later either: the next release's range starts at *this* release's head, so the + window is skipped, not deferred. - **Bitbucket caps `commits[]` at 5 per push.** Measured, that bites 0.6 % of - master pushes; it drops changes rather than mis-timing the ones it keeps. + master pushes. For changes it keeps it is harmless, but via the point above a + dropped commit that happens to be a release boundary costs its whole window. +- **Boundary commits are assumed to be push tips.** Range membership is decided + by push time, so every commit of a push lands on one side of the boundary. + That is exact when the boundary SHA is the tip of its push, which is what a + release cut normally is. Where it is not, commits sharing that push are + attributed to the neighbouring release. Likewise the GitOps release commit + must be a push tip to be found at all — a release split across two pushes + where Argo reports only the second revision leaves the first unresolvable. + Both go away with `image_ref`-based ranges. - **Ranges come from release notes today.** `deploy_commit_ranges` reads the compare links a release-note generator writes into the GitOps commit, so a release landed as a direct version bump is invisible to it. That is the view to diff --git a/src/riptide_collector/models.py b/src/riptide_collector/models.py index 771c254..2275690 100644 --- a/src/riptide_collector/models.py +++ b/src/riptide_collector/models.py @@ -29,6 +29,14 @@ class BitbucketEvent(Base): Index("ix_bitbucket_events_repo_full_name", "repo_full_name"), Index("ix_bitbucket_events_pr_id", "pr_id"), Index("ix_bitbucket_events_commit_sha", "commit_sha"), + # Serves the lead-time views: repo equality + branch equality + + # occurred_at range, which is exactly how a release window is looked up. + Index( + "ix_bitbucket_events_repo_branch_occurred", + "repo_full_name", + "branch_name", + "occurred_at", + ), Index( "ix_bitbucket_events_jira_keys_gin", "jira_keys", diff --git a/src/riptide_collector/views.py b/src/riptide_collector/views.py index 61ae2ec..c74e6c4 100644 --- a/src/riptide_collector/views.py +++ b/src/riptide_collector/views.py @@ -53,6 +53,19 @@ # compare link carrying the previous and the new App-repo SHA; those two bound # the range the deploy shipped. The LIKE prefilter keeps the regex off every # push payload in the table. +# +# Two assumptions, both true of the release generators this was built against +# and both stated in docs/dora-lead-time.md: +# +# - The GitOps release commit is a push *tip*, since `commit_sha` stores the +# push's toHash. A release split across two pushes where Argo only ever +# reports the second revision leaves the first push's bumps unresolvable. +# - Both boundary SHAs are push tips in the App repo, which is what makes the +# push-timestamp range below exact. Where a boundary is not the tip, the +# commits that share its push are attributed to the wrong side. +# +# Both disappear once senders report `pipeline_events.image_ref`: the range +# then comes from consecutive deploys of the same app, with no message parsing. DEPLOY_COMMIT_RANGES = r""" CREATE VIEW deploy_commit_ranges AS WITH release_commit AS ( @@ -70,9 +83,12 @@ bump AS ( SELECT app_name, environment, team, deployed_at, + -- Slugs may carry '_' and '.', so the class is wider than it looks; + -- the repo join below compares the slug exactly rather than with LIKE, + -- where '_' would act as a wildcard. regexp_matches( body, - 'repos/([a-z0-9-]+)/compare/diff\?targetBranch=([0-9a-f]{40})&sourceBranch=([0-9a-f]{40})', + 'repos/([a-z0-9._-]+)/compare/diff\?targetBranch=([0-9a-f]{40})&sourceBranch=([0-9a-f]{40})', 'g' ) AS m FROM release_commit @@ -90,7 +106,7 @@ FROM bump JOIN commit_sightings prev ON prev.commit_sha = bump.m[2] - AND prev.repo_full_name LIKE '%/' || bump.m[1] + AND split_part(prev.repo_full_name, '/', 2) = bump.m[1] JOIN commit_sightings curr ON curr.commit_sha = bump.m[3] AND curr.repo_full_name = prev.repo_full_name @@ -98,6 +114,12 @@ # First deploy only: a change reaches production once, even though the release # rolls out to every App of the service. +# +# The range is matched by push time, so every commit of a push belongs to one +# side of the boundary. That is exact while the boundary SHA is the push tip +# (see above) and it is why a boundary commit riptide never saw drops the whole +# release window rather than one commit — `deploy_commit_ranges` needs both +# ends. Those windows are absent from the metric, never counted as fast. LEAD_TIME_CHANGES = """ CREATE VIEW lead_time_changes AS SELECT diff --git a/tests/test_lead_time_views.py b/tests/test_lead_time_views.py index f961912..208479e 100644 --- a/tests/test_lead_time_views.py +++ b/tests/test_lead_time_views.py @@ -121,13 +121,17 @@ async def seeded( at=BASE, commits=[_commit(SHA_PREV, at=BASE, message="previous release head")], ) - # Three changes land on master, each committed two hours apart. + # Three changes land on master, each committed two hours apart. The + # first push carries two commits, as a PR merge of two commits does. await _push( session, delivery_id="p1", repo=APP_REPO, at=BASE + timedelta(hours=2), - commits=[_commit("b" * 40, at=BASE + timedelta(hours=2))], + commits=[ + _commit("b" * 40, at=BASE + timedelta(hours=1)), + _commit("9" * 40, at=BASE + timedelta(hours=2)), + ], ) await _push( session, @@ -235,7 +239,7 @@ async def test_only_master_commits_are_sighted(self, seeded: Any) -> None: # The feature-branch commit is absent: a change enters the release # stream when it lands on master, and counting both double-counts it. assert "e" * 40 not in rows - assert {SHA_PREV, "b" * 40, "c" * 40, SHA_HEAD} == set(rows) + assert {SHA_PREV, "b" * 40, "9" * 40, "c" * 40, SHA_HEAD} == set(rows) async def test_classification_columns(self, seeded: Any) -> None: async with seeded() as session: @@ -269,8 +273,8 @@ async def test_lead_time_measured_from_commit_to_first_deploy(self, seeded: Any) ) ).scalar_one() - # Committed at BASE+2h, first prod deploy at BASE+10h. - assert row == 8 + # Committed at BASE+1h, first prod deploy at BASE+10h. + assert row == 9 async def test_second_deploy_of_the_same_release_does_not_recount(self, seeded: Any) -> None: async with seeded() as session: @@ -301,7 +305,27 @@ async def test_range_excludes_the_previous_release_head(self, seeded: Any) -> No # The range is (previous head, this head]: the old head shipped last time. assert SHA_PREV not in shas - assert set(shas) == {"b" * 40, "c" * 40, SHA_HEAD} + assert set(shas) == {"b" * 40, "9" * 40, "c" * 40, SHA_HEAD} + + async def test_every_commit_of_a_push_is_counted(self, seeded: Any) -> None: + # Range membership is decided per push, so both commits of a + # two-commit push are attributed to the same release — and each keeps + # its own commit timestamp, an hour apart here. + async with seeded() as session: + rows = dict( + ( + await session.execute( + text( + "SELECT commit_sha, extract(epoch FROM lead_time)/3600 " + "FROM lead_time_changes WHERE environment = 'prod' " + " AND commit_sha IN (:a, :b)" + ), + {"a": "b" * 40, "b": "9" * 40}, + ) + ).all() + ) + + assert rows == {"b" * 40: 9, "9" * 40: 8} async def test_environments_are_measured_separately(self, seeded: Any) -> None: async with seeded() as session: @@ -317,7 +341,7 @@ async def test_environments_are_measured_separately(self, seeded: Any) -> None: ).all() ) - assert rows == {"intg": 6, "prod": 8} + assert rows == {"intg": 7, "prod": 9} async def test_default_metric_filter_keeps_only_real_changes(self, seeded: Any) -> None: async with seeded() as session: @@ -336,5 +360,6 @@ async def test_default_metric_filter_keeps_only_real_changes(self, seeded: Any) ) # The merge commit and the release-plugin commit are artifacts of - # shipping, not changes that were shipped. - assert shas == ["b" * 40] + # shipping, not changes that were shipped. Both commits of the + # two-commit push survive. + assert sorted(shas) == sorted(["b" * 40, "9" * 40])