diff --git a/AGENTS.md b/AGENTS.md index 307b85f..6c9acb9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,70 +3,68 @@ ## Commands ```bash -uv sync # install deps (creates .venv) -uv run pytest # all tests (uses testcontainers → needs Docker/OrbStack) -uv run pytest --cov # with coverage gate (fail_under = 85, branch coverage) -uv run pytest tests/test_parsers.py # one file -uv run pytest -k test_revert # one test by keyword -uv run ruff check . && uv run ruff format --check . # lint + format check -uv run ruff format . # auto-format -uv run basedpyright # type-check (strict mode for src/) -RIPTIDE_DB_URL=... uv run alembic upgrade head # apply migrations -RIPTIDE_DB_URL=... uv run alembic downgrade base # tear down -podman-compose up # local dev: Postgres + migrations + app on :8000 +uv sync # deps → .venv +uv run pytest # tests (testcontainers → needs Docker/OrbStack) +uv run pytest --cov # coverage gate, fail_under 85, branch +uv run ruff check . && uv run ruff format --check . +uv run basedpyright # strict for src/ +RIPTIDE_DB_URL=... uv run alembic upgrade head # / downgrade base +podman-compose up # Postgres + migrations + app on :8000 ``` -If `docker ps` fails, ask the user to start OrbStack. +`docker ps` fails → ask the user to start OrbStack. ## Architecture invariants -- **Append-only ingestion.** Every webhook handler does `INSERT … ON CONFLICT (delivery_id) DO NOTHING`; never `UPDATE` or `DELETE` event rows. `delivery_id` is the per-source dedup key, so retries are idempotent. -- **Raw payload always stored.** `payload JSONB` keeps the full request body even if fields are extracted into typed columns. Don't drop fields you don't currently use. -- **`riptide.json` is config, not data.** `openshift/collector/riptide.json` (in-repo sample) declares teams (name + `group_email`) and org-wide automation rules; edits go through PRs and the pod hot-reloads via mtime in `RiptideConfigStore.maybe_reload()`. Never propose moving it into Postgres. -- **Per-team bearer keys live in a separate file**, mounted in production from the `riptide-collector-team-keys` Secret (never committed); `openshift/collector/team-keys.json` is a dev sample with deterministic test hashes (raw dev bearers in `compose.yaml`). Stored as sha256, hot-reloaded by `TeamKeysStore` like the config. The bearer **is** the team identity — every webhook is tagged `team = caller_team`. -- **No `service` column, no `service_id` on the wire.** Per-source aggregations group by `repo_full_name` / `pipeline_name` / `app_name` / `repo`; org-wide rollups by `team`. Identifiers are lowercased at ingest (`commit_sha`, `revision`, `repo_full_name`, `branch_name`, `repo`) so joins are case-stable. Never propose a unified `service` column or `service_id` — it served only single-pane labelling and was dropped. -- **`automation` is org-wide.** Bot definitions live at the config root, not per team. Config is the last resort, not the first: prefer what the upstream payload already states (the acting user's `type == "SERVICE"` — `actor` on push and reviewer events, `pullRequest.author.user` on PR lifecycle ones → `automation_source = "service-account"`, ranked above the `*-bot` name guess) and what senders declare about themselves (`reviewer_handle` / `actor_handle` + kind). Only accounts nobody reports get a config entry. -- **Metrics are computed on read, not at ingest.** No aggregation tables or scheduled rollup jobs in v1. Schema additions preserve raw events; new metrics are SQL against existing rows or future materialized views. -- **Commit SHA joins Bitbucket↔Pipeline; Argo CD joins on the image reference.** `bitbucket_events.commit_sha = pipeline_events.commit_sha` is deterministic (App-repo SHA both sides). `argocd_events.revision` is the **GitOps-repo SHA** — proven empirically, four Apps for one service share one revision — so it does NOT match the other two. Image **tags are not commit SHAs** either (measured in production: 0 of 4 936 image refs, all semver) — never build correlation on parsing a SHA out of a tag. The contract is the **full image reference**: senders report `pipeline_events.image_ref` (`registry/path:tag`), Argo CD stores the same strings in `payload->'images'` from `.app.status.summary.images`, and the join is exact. Historical rows predating `image_ref` fall back to the release-note correlator in `docs/correlating-deploys-to-commits.md`. Never propose `service_id` or hand-coded name mappings to fix correlation. -- **`change_type` lives on Bitbucket events only.** Don't denormalise it onto pipeline / Argo rows; join Pipeline rows via `commit_sha` and Argo rows via `payload->'images'` ↔ `pipeline_events.image_ref` at read time. -- **CI events are source-tagged, not source-routed.** All pipeline events from any CI (Jenkins, Tekton, …) land in the single `pipeline_events` table via `POST /webhooks/pipeline`, distinguished by the `source` column. Do not add per-CI tables or endpoints. The dedup key is `source#pipeline_name#run_id#phase`. -- **Noergler events carry finops + reviewer-precision only.** The `noergler_events` table is `event_type`-discriminated (`pr_completed` | `feedback`; historical rows may carry the pre-0002 `completed`) and is fed by `POST /webhooks/noergler` from optional noergler instances. Do not re-emit PR lifecycle from noergler — `bitbucket_events` already covers open / merged / declined. Dedup keys: `pr_completed##` and `feedback##`. `pr_completed` is also the source for PR diff size and for `reviewer_handle`, the bot's own account. -- **Senders verify reachability + bearer at startup via `GET /auth/ping`** — authenticated, returns `{"status":"ok","team":""}`, so a wrong token fails fast. Never reuse `/health` (unauth liveness) or `/ready` (unauth readiness) for it; those answer different questions. -- **`modified_at` has a Postgres trigger** (`riptide_set_modified_at`), not just SQLAlchemy `onupdate`. Raw-SQL updates also bump it. Keep the trigger when changing migrations. -- **Database is external.** `riptide-collector` does NOT manage Postgres. Do not add a Postgres Deployment to `openshift/`. -- **Pyright strict for `src/`, standard for `tests/` and `migrations/`.** New code under `src/` must satisfy strict mode — no `Any` leaks; narrow `Optional`s with `isinstance` or helpers like `_as_dict()` in `routers/bitbucket.py`. +- **Append-only.** Handlers `INSERT … ON CONFLICT (delivery_id) DO NOTHING`. Never `UPDATE` / `DELETE` event rows. `delivery_id` = per-source dedup key, so retries are idempotent. +- **Raw payload always stored** in `payload JSONB`, whole body, even for fields already extracted into columns. Don't drop unused fields. +- **`riptide.json` is config, not data.** Teams + org-wide automation rules. Edits via PR, pod hot-reloads by mtime. Never move it into Postgres. +- **Team keys are a separate file**, production-mounted from a Secret, never committed. Stored sha256, hot-reloaded. The bearer **is** the team identity — every webhook tagged `team = caller_team`. +- **No `service` column, no `service_id` on the wire.** Aggregate per source by `repo_full_name` / `pipeline_name` / `app_name` / `repo`, org-wide by `team`. Join identifiers are lowercased at ingest (`commit_sha`, `revision`, `repo_full_name`, `branch_name`, `repo`) → case-stable. It served only single-pane labelling and was dropped; never propose it again. +- **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. +- **`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`. +- **Noergler carries finops + reviewer-precision only.** `event_type` ∈ `pr_completed` | `feedback` (historical rows: pre-0002 `completed`). Never re-emit PR lifecycle — `bitbucket_events` covers open / merged / declined. Dedup keys `pr_completed##`, `feedback##`. `pr_completed` is also the source for PR diff size (Bitbucket webhooks carry none) and for the reviewer's own account. +- **Senders verify at startup via `GET /auth/ping`** — authenticated, returns the caller's team, so a wrong token fails fast. Never reuse `/health` (unauth liveness) or `/ready` (unauth readiness). +- **`modified_at` has a Postgres trigger** (`riptide_set_modified_at`), not just SQLAlchemy `onupdate`, so raw-SQL updates bump it too. Keep the trigger when changing migrations. +- **Database is external.** Never add a Postgres Deployment to `openshift/`. ## Repo conventions -- **Layering.** Routers do HTTP + auth + dispatch only. Payload extraction lives in `parsers_.py` (e.g. `parsers_bitbucket.py`) as pure functions returning a typed `*EventDraft` — no HTTP, no DB, no config. The router computes config-derived fields (`automation_source`) and persists. Never put extraction in routers, and keep JSON-shape coercion helpers with the extractor that uses them. -- Single flat package `riptide_collector`, not a namespace package. Future suite components (`riptide-api`, `riptide-dashboard`) get their own top-level package — leave room for them. -- Webhook routers are factories returning an `APIRouter`, wired in `main.py::create_app`. Bitbucket takes the config for automation detection (`make_router(config, session_factory, auth_dep)`); Pipeline, ArgoCD and Noergler take just `(session_factory, auth_dep)`. Pass the config only when a router needs `automation` rules or team metadata. -- Pydantic schemas: **strict** for `/webhooks/pipeline` and `/webhooks/argocd` (we own the contract — invalid payloads must 422); **permissive raw-dict parsing** for Bitbucket (its payload shapes vary; we best-effort extract). -- Use `_as_dict()` / `_as_list()` helpers in `routers/bitbucket.py` to coerce arbitrary JSON shapes — basedpyright strict won't accept chained `.get()` on `Optional[dict]`. -- Tests use real Postgres via testcontainers, never SQLite. The `client` fixture in `tests/conftest.py` depends on `session_factory` which truncates tables per test. -- `.pre-commit-config.yaml` runs ruff + basedpyright + uv-lock-check; expect CI to enforce the same. +- **Layering.** Routers: HTTP + auth + dispatch + config-derived fields + persist. Extraction: `parsers_.py`, pure functions returning a typed `*EventDraft`, no HTTP / DB / config. Keep JSON-coercion helpers beside the extractor using them. Never extract in a router. +- Pass the config to a router only when it needs `automation` rules or team metadata. +- Schemas **strict** for `/webhooks/pipeline`, `/webhooks/argocd`, `/webhooks/noergler` — we own those contracts, invalid payloads must 422. Bitbucket is permissive raw-dict parsing; its shapes vary. +- Optional fields: accept `""` as absent. A templated-but-unset param arrives empty far more often than missing, and rejecting it drops the whole event. +- Coerce arbitrary JSON with the `_as_dict()` / `_as_list()` helpers — basedpyright strict rejects chained `.get()` on `Optional[dict]`. +- Pyright strict for `src/`, standard for `tests/` and `migrations/`. No `Any` leaks in `src/`. +- Single flat package `riptide_collector`. Future suite components get their own top-level package. +- Tests: real Postgres via testcontainers, never SQLite. Per-test truncation via the `session_factory` fixture. +- `.pre-commit-config.yaml` = ruff + basedpyright + uv-lock-check; CI enforces the same. ## Logging & Splunk -- **One JSON object per line, on stdout.** Splunk Connect for Kubernetes tails the container log and auto-extracts via `KV_MODE=json` for sourcetype `riptide:collector:json` (pod annotation in `openshift/collector/deployment.yaml`). -- **Stdlib loggers (uvicorn, sqlalchemy, alembic) are bridged through structlog.** Do NOT add separate logging handlers or re-init `logging.basicConfig` — `configure_logging()` in `logging_config.py` is the single entry point. -- **Splunk-reserved names are forbidden as kwargs**: `source`, `sourcetype`, `host`, `index`, `time`, `_time`, `_raw`, `event`. CI vendor is `ci_system`, the structlog event name is `msg`, severity is `log_level`. `_strip_reserved` namespaces accidents under `splunk_` as a safety net — never rely on it, pick the right name. -- **Field naming:** generic names that mean the same across sources (`event_type`, `status`, `phase`, `delivery_id`, `team`, `repo`, `commit_sha`). Never pre-namespace with the source (`noergler_event_type`) — `webhook_source` already disambiguates in `stats by webhook_source, event_type`. Namespace only when two sources genuinely mean different things by one word and would collide in a panel. -- **Exactly one `msg=webhook_processed` per request**, with `webhook_source ∈ {bitbucket,pipeline,argocd,noergler}`, `outcome ∈ {accepted,deduped,ignored,skipped}`, `delivery_id`, `team`, plus source-specific fields (`app`, `revision`, `phase` for argocd). Include `delivery_id` even on `ignored`/`skipped` so triage has a key. -- **`outcome=deduped`** is detected via `RETURNING delivery_id` on the `INSERT ... ON CONFLICT DO NOTHING` — a `None` scalar means the row already existed. Preserve this when adding new sources. -- **Persist failures**: wrap the `async with session_factory()` block in `try/except Exception: logger.exception("webhook_persist_failed", ...); raise`. Never swallow. -- **Access log**: `access_log` middleware in `main.py` emits `msg=http_request` with `request_id`, `method`, `path`, `status_code`, `duration_ms`. `request_id` is bound to contextvars so every log in the request inherits it. `/health` and `/ready` are silenced; uvicorn.access stays at WARNING. -- The Splunk `props.conf` stanza is owned by the platform team; a copy for reference lives in [`docs/splunk-props.conf`](docs/splunk-props.conf). +- **One JSON object per line on stdout**, auto-extracted by Splunk (`KV_MODE=json`, sourcetype `riptide:collector:json`). +- **`configure_logging()` is the single entry point**; stdlib loggers (uvicorn, sqlalchemy, alembic) are bridged through structlog. Never add handlers or re-init `logging.basicConfig`. +- **Splunk-reserved kwargs are forbidden**: `source`, `sourcetype`, `host`, `index`, `time`, `_time`, `_raw`, `event`. CI vendor → `ci_system`, event name → `msg`, severity → `log_level`. `_strip_reserved` is a safety net, not a licence. +- **Field names generic across sources** (`event_type`, `status`, `phase`, `delivery_id`, `team`, `repo`, `commit_sha`). Never pre-namespace with the source — `webhook_source` already disambiguates. Namespace only on a genuine collision of meaning. +- **Exactly one `msg=webhook_processed` per request**: `webhook_source` ∈ {bitbucket,pipeline,argocd,noergler}, `outcome` ∈ {accepted,deduped,ignored,skipped}, `delivery_id`, `team`, plus source-specific fields. Include `delivery_id` even on ignored / skipped so triage has a key. +- **`outcome=deduped`** comes from `RETURNING delivery_id` — a `None` scalar means the row existed. Preserve when adding sources. +- **Persist failures**: `try/except Exception: logger.exception("webhook_persist_failed", …); raise`. Never swallow. +- **Access log** binds `request_id` to contextvars so every log in the request inherits it. `/health` and `/ready` silenced; uvicorn.access at WARNING. +- Splunk `props.conf` is owned by the platform team; reference copy in [`docs/splunk-props.conf`](docs/splunk-props.conf). ## OpenShift layout -`openshift/` is suite-level, one directory per component (`openshift/collector/`). Adding a component: create `openshift//` with its own `kustomization.yaml`, add it to `resources:` in `openshift/kustomization.yaml`, give every container explicit cpu+memory `requests` AND `limits` (no exceptions), and set `runAsNonRoot: true` + `readOnlyRootFilesystem: true` with no fixed `runAsUser` (OpenShift assigns a random UID per project). +`openshift/` is suite-level, one directory per component. New component → own `openshift//kustomization.yaml`, added to `resources:` in `openshift/kustomization.yaml`. Every container: explicit cpu+memory `requests` AND `limits`, no exceptions. `runAsNonRoot: true`, `readOnlyRootFilesystem: true`, never a fixed `runAsUser` — OpenShift assigns a random UID per project. -## What's intentionally out of v1 +## Out of v1 -If asked to add these, push back unless the user is explicit: -- Change failure rate / failed deployment recovery time (DORA's current term, formerly MTTR) — no reliable incident source yet; schema reserves room for rollback-proxy detection -- Backfill workers (forward-only ingestion only) -- Aggregation API or metric endpoints (collector ingests; reads are SQL or future siblings) -- Helm chart (Kustomize is enough for v1) -- Postgres deployment manifests +Push back unless the user is explicit: +- Change failure rate / failed deployment recovery time — no reliable incident source; schema leaves room for rollback-proxy detection +- Backfill workers (ingestion is forward-only) +- Aggregation API or metric endpoints (reads are SQL, or a future sibling component) +- Helm chart (Kustomize suffices) +- Postgres manifests diff --git a/README.md b/README.md index 5502d71..67ea6e1 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ Ingestion service for the **riptide** DevOps delivery-metrics suite. - [Overview](#overview) - [What it collects](#what-it-collects) +- [Reading the event tables](#reading-the-event-tables) - [Metrics](#metrics) - [Quickstart (local)](#quickstart-local) - [Database](#database) @@ -38,6 +39,31 @@ Raw events from: …stored append-only in Postgres for later metric computation by other suite components or ad-hoc SQL. +## Reading the event tables + +Before writing a query, know what an event means. Two readings look obvious +and are wrong, both measured on 3.5 months of production data: + +**`repo:refs_changed` is a ref-movement log, not developer activity.** It says +master now points at a different commit — it does not say a person pushed. In +that dataset, 16 295 of these events were on `master` and only 1 210 carried a +human name; every one of those was a merge commit (`Pull request #NNN: …`), +already counted on the PR side as `pr:merged`. The other 15 000 were release +tooling: `[maven-release-plugin] prepare for next development iteration`, +`[gradle-release] …`, a component-version job, and Renovate landing updates. +Build per-author or per-repo activity views on PR events; use +`repo:refs_changed` for what moved, and for revert detection. + +**`change_type` only means something where a branch prefix exists.** It is +parsed from `branch_name`, so a push to `master` has nothing to classify and +falls to `other`. Computed over all events it read 83 % `other`, which says +nothing about the work: restrict it to `pr:opened` / `pr:merged` rows, where +the source branch carries the prefix. + +The general rule: an event's name describes what the git host did, not who did +it or why. Check `author` and the commit message before reading intent into a +row count. + ## Metrics riptide-collector ingests; **metrics are computed on read** as SQL queries @@ -149,7 +175,7 @@ because there's no clock-start to subtract from in the first place. | **PR size** | `lines_added`, `lines_removed`, `files_changed` on `noergler_events` (`event_type = 'pr_completed'`), joined to Bitbucket PRs on `pr_key` = `'#'`. The same columns exist on `bitbucket_events` but are always NULL: Bitbucket DC webhooks carry no diff stats, and fetching them would put an outbound REST call in the ingest path. Covers the repos noergler reviews. | | **Revert rate** | `COUNT(*) WHERE is_revert = true` over total commits — a free, weak Change-Failure-Rate proxy. | | **Hotfix rate** | `COUNT(*) WHERE change_type = 'hotfix'` over total deploys per window — operational-pain signal. | -| **Change mix** | Distribution of `change_type` (feature / bugfix / hotfix / chore / refactor / docs / other) per team per week. | +| **Change mix** | Distribution of `change_type` (feature / bugfix / hotfix / chore / refactor / docs / other) per team per week, over `pr:opened` / `pr:merged` rows only — a push to `master` has no prefix to classify and would swamp the distribution with `other`. | | **Tickets per deploy** | `COUNT(DISTINCT unnest(jira_keys))` per deploy — small-batch indicator. Jira keys are extracted at write time from PR title, description, branch name, and commit messages via regex `[A-Z][A-Z0-9]+-\d+`, deduplicated, GIN-indexed. | | **Untracked-work rate** | `COUNT(*) WHERE jira_keys = '{}'` over merged PRs — process-compliance signal. | | **Per-ticket flow** | `WHERE 'ABC-1234' = ANY(jira_keys)` returns every event for a ticket across Bitbucket / pipeline / Argo (joined via commit_sha). |