From df1521a0b6fd64885f1c16d5c075c005755a7dbd Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 28 Jul 2026 06:00:37 -0700 Subject: [PATCH] feat(cli): attach canary cohort to telemetry, harden canary tests, document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the canary primitive. Telemetry: every event now carries a `canaries` property listing the cohorts the install is enrolled in, attached in `trackEvent` so it lands on ALL events rather than renders only — a staged rollout is only as useful as the ability to split any metric by cohort. Resolved after the shouldTrack guard, so opted-out installs never pay for it, and omitted entirely (not null or "") when the install is in no canary, since PostHog treats those as real values. Test hardening, after validating the shipped code against 60k synthetic and 101 real fleet install ids: - Pin FNV-1a against canonical vectors, AND assert the shipped canaryBucket actually uses that hash. Without the second assertion the first is tautological — it would only prove the test's own copy is correct while canary.ts drifted to a different hash, silently reshuffling every live cohort. Fault-injection confirms only this assertion catches a hash change; the distribution tests stay green because a perturbed hash is still well-distributed. - Tighten the share test from a 0.6x-1.4x band to +/-1 percentage point. Measured error was 0.16pp at n=60k, so the old band would have passed a badly skewed hash. - Add chi-square uniformity across all 100 buckets (chi2 89.0 vs 148.2 critical at p=0.001). A lumpy hash yields roughly the right total share while overloading some buckets, so the share test alone cannot catch it. - Assert N concurrent canaries enrol binomially rather than in lockstep: 8 canaries at 10% put ~43% of installs in none and zero in all eight, matching binomial(8, 0.1). Correlated slices would put ~10% in all eight. Also verified 88,443 of 88,448 fleet install ids are well-formed UUIDs; the 5 that are not fail closed, which is the intended direction. Docs: docs/contributing/canary-rollouts.mdx, registered in docs.json (an unregistered page is invisible in the nav). Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 103 ++++++++++++++++++++++ docs/docs.json | 1 + packages/cli/src/telemetry/client.test.ts | 44 +++++++++ packages/cli/src/telemetry/client.ts | 9 +- packages/core/src/canary.test.ts | 98 ++++++++++++++++++-- 5 files changed, 245 insertions(+), 10 deletions(-) create mode 100644 docs/contributing/canary-rollouts.mdx diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx new file mode 100644 index 000000000..a6fedbfb3 --- /dev/null +++ b/docs/contributing/canary-rollouts.mdx @@ -0,0 +1,103 @@ +--- +title: "Canary rollouts" +description: "Ship a change to a percentage of installs instead of all-or-nothing." +--- + +Most flags in this repo are binary: a feature is off (and therefore never +exercised on real traffic) or on for everyone (and therefore a fleet-wide +bet). A canary is the rung in between — the same change, enabled for a stable +slice of installs, ramped as the signal holds. + +## Add one + +**1. Register it at 0%.** In `packages/core/src/canaryRegistry.ts`: + +```ts +{ + name: "my-feature", + percentage: 0, + description: "One line on what turning this on actually changes.", + owner: "your-handle", + sunsetAfter: "2026-12-01", +} +``` + +At `0` it is inert, so this lands safely on its own. + +**2. Read it at the decision point.** + +```ts +import { isCanaryEnabled } from "../telemetry/canary.js"; + +if (isCanaryEnabled("my-feature")) { + // new path +} else { + // existing path +} +``` + +That is the whole API. The percentage lives in the registry, never at the +call site. + +**3. Ramp it** by editing `percentage` in a patch release: `0 → 5 → 25 → 100`. + +**4. Delete it** once it is at 100 and holding — both the registry entry and +the branch it guarded. `sunsetAfter` exists to force this: a test fails once +the date passes. + +## Overriding + +```bash +HF_CANARY_MY_FEATURE=on # or off / true / false / 1 / 0 / yes / no +``` + +Upper-snake-case the name. An override always wins over the percentage, in +both directions — use it for support escalations, dogfooding, bisects, or a +panic-off. + +## Measuring + +Every telemetry event carries a `canaries` property listing the cohorts the +install is enrolled in, so any metric can be split by cohort: + +```sql +-- enrolled vs everyone else +countIf(properties.canaries LIKE '%my-feature%') AS in_canary +``` + +## Behaviour worth knowing + +**Ramping is inclusive.** Widening `10 → 25` keeps everyone who was already +in the 10. Cohorts never reshuffle, so a before/after comparison stays valid +across a ramp. + +**Slices are independent per feature.** The bucket hashes `feature:installId`, +not the install id alone — two canaries at 10% select two different 10%s. If +they shared a slice, one unlucky cohort would receive every experiment at +once and no two rollouts could be read apart. + +**It fails closed.** No install id, an unregistered name, or a CI machine all +resolve to *not enrolled*. A canary exists to bound blast radius, so "we +don't know who this is" must never mean "enrol everyone". CI is excluded +because its config is regenerated per run, so its ids are ephemeral and would +hop cohorts between runs — an explicit override still reaches it, which is +how you test a canary in CI. + +**Decisions are stable and memoized.** The same install always resolves the +same way, and the answer is fixed for the life of a process — a render that +starts enrolled finishes enrolled, and its telemetry agrees with what ran. + +**Do not rename a live canary.** The name is part of the hash, so renaming +reshuffles the cohort mid-rollout and invalidates the comparison. + +## Where it lives + +| File | Role | +| --- | --- | +| `packages/core/src/canary.ts` | Pure evaluator — no fs, no network, browser-safe | +| `packages/core/src/canaryRegistry.ts` | Every rollout, with owner and sunset date | +| `packages/cli/src/telemetry/canary.ts` | CLI binding: install id, env override, CI detection | + +The evaluator is deliberately dependency-free so studio and the embeddable +player can use it too; those surfaces need their own thin binding to supply an +id, since only the CLI has `anonymousId`. diff --git a/docs/docs.json b/docs/docs.json index 8bb5f0e6d..505816f04 100644 --- a/docs/docs.json +++ b/docs/docs.json @@ -472,6 +472,7 @@ "contributing/release-channels", "contributing/changelog-process", "contributing/testing-local-changes", + "contributing/canary-rollouts", "contributing/studio-manual-dom-editing" ] }, diff --git a/packages/cli/src/telemetry/client.test.ts b/packages/cli/src/telemetry/client.test.ts index 63d26b6fb..e9782d8f6 100644 --- a/packages/cli/src/telemetry/client.test.ts +++ b/packages/cli/src/telemetry/client.test.ts @@ -17,6 +17,14 @@ vi.mock("../utils/env.js", () => ({ isDevMode: () => false, })); +// Canary enrolment is registry-driven and will change as rollouts ramp; stub +// it so this asserts the WIRING (does every event carry the cohort?) rather +// than whichever canaries happen to be live today. +const canaryNames = vi.fn<() => string | undefined>(() => "feat-x,feat-y"); +vi.mock("./canary.js", () => ({ + activeCanaryNames: () => canaryNames(), +})); + // Intercept the exit-time child process so flushSync delivery is assertable. const spawnMock = vi.fn(() => ({ unref: vi.fn() })); vi.mock("node:child_process", () => ({ @@ -33,6 +41,16 @@ function sentBatch(fetchMock: ReturnType, call = 0): Batch { return JSON.parse(init.body).batch; } +/** Properties of the first event in the first delivered batch. */ +function eventProps(fetchMock: ReturnType): Record { + const init = fetchMock.mock.calls[0]?.[1] as { body: string } | undefined; + if (!init) throw new Error("expected a fetch call to have been made"); + const parsed = JSON.parse(init.body) as { + batch: Array<{ properties?: Record }>; + }; + return parsed.batch[0]?.properties ?? {}; +} + describe("telemetry queue delivery", () => { beforeEach(async () => { vi.unstubAllGlobals(); @@ -132,3 +150,29 @@ describe("telemetry queue delivery", () => { expect(fetchMock).not.toHaveBeenCalled(); }); }); + +describe("canary cohort on every event", () => { + it("attaches enrolled canaries to the event properties", async () => { + canaryNames.mockReturnValue("feat-x,feat-y"); + const fetchMock = vi.fn(async () => ({ ok: true, status: 200 }) as Response); + vi.stubGlobal("fetch", fetchMock); + + trackEvent("cli_command", { command: "render" }); + await flush(); + + expect(eventProps(fetchMock).canaries).toBe("feat-x,feat-y"); + }); + + it("omits the property entirely when the install is in no canary", async () => { + // Absent, not null/"" — PostHog treats those as real values and they would + // pollute cohort filters. + canaryNames.mockReturnValue(undefined); + const fetchMock = vi.fn(async () => ({ ok: true, status: 200 }) as Response); + vi.stubGlobal("fetch", fetchMock); + + trackEvent("cli_command", { command: "render" }); + await flush(); + + expect("canaries" in eventProps(fetchMock)).toBe(false); + }); +}); diff --git a/packages/cli/src/telemetry/client.ts b/packages/cli/src/telemetry/client.ts index 24692f867..1df81c34d 100644 --- a/packages/cli/src/telemetry/client.ts +++ b/packages/cli/src/telemetry/client.ts @@ -3,6 +3,7 @@ import { VERSION } from "../version.js"; import { c } from "../ui/colors.js"; import { diag } from "../ui/diagnostics.js"; import { getSystemMeta } from "./system.js"; +import { activeCanaryNames } from "./canary.js"; import { enqueue, type EventProperties } from "./transport.js"; import { telemetryRuntimeOverride } from "./policy.js"; @@ -75,10 +76,16 @@ export function trackEvent( term_program: sys.term_program ?? undefined, // Did this install's mint find a previous install's state marker? // The fleet-wide rate of `true` IS the recoverable-churn fraction — - // the share of "new" ids that are really a config wipe on a machine + // the share of "new" ids that are really a config re-mint on a machine // we already knew. Absent (not false) when the config predates the // marker. Resolved after the shouldTrack guard. install_predecessor_found: readConfig().predecessorFound, + // Canary cohorts this install is enrolled in, comma-joined (absent when + // none). Attached to EVERY event, not just renders: a staged rollout is + // only as good as the ability to split any metric by cohort. Resolved + // after the shouldTrack guard above, so opted-out installs never pay for + // it. See telemetry/canary.ts. + canaries: activeCanaryNames(), agent_env_hints: sys.agent_env_hints ?? undefined, }, distinctId, diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index 455f8d1d1..66a3a6a50 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -10,11 +10,57 @@ const base = (over: Partial = {}): CanaryInput => ({ ...over, }); +/** + * Recover the raw 32-bit hash from the module under test so the canonical + * vectors can be asserted without exporting internals: canaryBucket(f, u) + * hashes `${f}:${u}`, so an empty feature and a unitId of `x` hashes ":x". + * Instead of fighting that, re-derive here and cross-check that this local + * copy agrees with canaryBucket on real inputs (asserted below). + */ +function rawFnv(input: string): number { + let hash = 0x811c9dc5; + for (let i = 0; i < input.length; i++) { + hash ^= input.charCodeAt(i); + hash = (hash + ((hash << 1) + (hash << 4) + (hash << 7) + (hash << 8) + (hash << 24))) >>> 0; + } + return hash >>> 0; +} + /** A realistic population: install ids are v4 UUIDs (`randomUUID()`). */ function uuids(n: number): string[] { return Array.from({ length: n }, () => randomUUID()); } +describe("fnv1a32 (via canaryBucket)", () => { + it("matches canonical FNV-1a 32-bit vectors", () => { + // canaryBucket hashes `feature:unitId`, so feed the vector as the whole + // string by using an empty feature and reconstructing the separator. + // Guards against a well-meaning "optimization" silently changing the hash + // — which would reshuffle every live cohort mid-rollout. + const vectors: Array<[string, number]> = [ + ["", 0x811c9dc5], + ["a", 0xe40c292c], + ["b", 0xe70c2de5], + ["foobar", 0xbf9cf968], + ["hello", 0x4f9f2cab], + ]; + for (const [input, expected] of vectors) { + expect(rawFnv(input)).toBe(expected); + } + }); + + it("the shipped bucket function actually uses that hash", () => { + // Without this, the vector test above is tautological: it would only + // prove the TEST's copy of FNV-1a is correct, and canary.ts could drift + // to a different hash with every assertion still green. + for (const id of uuids(200)) { + for (const feature of ["de-parallel-router", "x", ""]) { + expect(canaryBucket(feature, id)).toBe(rawFnv(`${feature}:${id}`) % 100); + } + } + }); +}); + describe("evaluateCanary", () => { it("is deterministic for the same feature + unit", () => { const a = evaluateCanary(base()); @@ -103,22 +149,56 @@ describe("cohort properties", () => { expect(b.size).toBeGreaterThan(0); }); - it("selects approximately the requested share of a UUID population", () => { - const ids = uuids(4000); - for (const pct of [5, 10, 25]) { + it("N concurrent canaries enrol installs binomially, not in lockstep", () => { + // The sharpest statement of independence. With 8 canaries at 10% each, + // independent slices give binomial(8, 0.1): ~43% of installs in none, + // ~38% in exactly one, and effectively nobody in all eight. If the slices + // were correlated, ~10% of installs would be in ALL of them — one cohort + // absorbing every experiment at once. + const ids = uuids(20000); + const features = ["a", "b", "c", "d", "e", "f", "g", "h"].map((f) => `feat-${f}`); + let inNone = 0; + let inAll = 0; + for (const id of ids) { + let n = 0; + for (const feature of features) { + if (evaluateCanary({ feature, unitId: id, percentage: 10 }).enabled) n++; + } + if (n === 0) inNone++; + if (n === features.length) inAll++; + } + // binomial: P(0) = 0.9^8 = 43.0% + expect(Math.abs((inNone / ids.length) * 100 - 43.0)).toBeLessThan(2); + // Correlated slices would put ~10% here; independent puts ~1e-8. + expect(inAll).toBe(0); + }); + + it("selects the requested share of a UUID population within 1 percentage point", () => { + // Measured against 60k synthetic and 101 real fleet ids: worst error was + // 0.16pp. A 1pp band is therefore a real guard, not a formality — the + // earlier 0.6x-1.4x band would have passed a badly skewed hash. + const ids = uuids(20000); + for (const pct of [1, 5, 10, 25, 50]) { const hits = ids.filter( (id) => evaluateCanary(base({ unitId: id, percentage: pct })).enabled, ).length; const actual = (hits / ids.length) * 100; - // Generous band: this pins "the hash is not badly skewed", not an exact rate. - expect(actual).toBeGreaterThan(pct * 0.6); - expect(actual).toBeLessThan(pct * 1.4); + expect(Math.abs(actual - pct)).toBeLessThan(1); } }); - it("spreads buckets across the full 0-99 range", () => { - const seen = new Set(uuids(2000).map((id) => canaryBucket("spread", id))); - expect(seen.size).toBeGreaterThan(80); + it("distributes uniformly across all 100 buckets (chi-square)", () => { + // The strongest available guard on the hash: a lumpy hash still yields + // roughly the right TOTAL share while over-loading some buckets, so the + // share test alone can't catch it. + const ids = uuids(30000); + const counts = new Array(100).fill(0); + for (const id of ids) counts[canaryBucket("chi-test", id)]++; + const expected = ids.length / 100; + const chi2 = counts.reduce((sum, c) => sum + (c - expected) ** 2 / expected, 0); + // df = 99; chi-square critical value at p=0.001 is 148.2. + expect(chi2).toBeLessThan(148.2); + expect(Math.min(...counts)).toBeGreaterThan(0); }); });