diff --git a/packages/cli/src/telemetry/canary.test.ts b/packages/cli/src/telemetry/canary.test.ts index 4771c0c14..384a9d33d 100644 --- a/packages/cli/src/telemetry/canary.test.ts +++ b/packages/cli/src/telemetry/canary.test.ts @@ -142,6 +142,9 @@ describe("telemetry opt-out is canary opt-out", () => { "$feature/canary-test-alpha": "false", "$feature/canary-test-gamma": "false", "$feature/canary-test-beta": "false", + canary_reason_test_alpha: "telemetry_opt_out", + canary_reason_test_gamma: "telemetry_opt_out", + canary_reason_test_beta: "telemetry_opt_out", }); }); }); @@ -228,6 +231,9 @@ describe("CLI canary binding", () => { "$feature/canary-test-alpha": "true", "$feature/canary-test-gamma": expect.stringMatching(/^(true|false)$/), "$feature/canary-test-beta": "false", + canary_reason_test_alpha: "in_cohort", + canary_reason_test_gamma: expect.stringMatching(/^(in_cohort|out_of_cohort)$/), + canary_reason_test_beta: "out_of_cohort", }); __resetCanaryCacheForTests(); @@ -235,3 +241,36 @@ describe("CLI canary binding", () => { expect(canaryEventProperties()["$feature/canary-test-alpha"]).toBe("false"); }); }); + +// End of the wire: the CLI binding computes the reason and used to drop it. +describe("reason reaches the event properties", () => { + it("pairs every canary's assignment with its reason", () => { + const props = canaryEventProperties(); + expect(props["$feature/canary-test-alpha"]).toBe("true"); + expect(props["canary_reason_test_alpha"]).toBe("in_cohort"); + expect(props["canary_reason_test_beta"]).toBe("out_of_cohort"); + }); + + it("reports an explicit override as forced, not as a cohort roll", () => { + process.env.HF_CANARY_TEST_BETA = "on"; + const props = canaryEventProperties(); + expect(props["$feature/canary-test-beta"]).toBe("true"); + // Same assignment a real enrolment would produce — the reason is the only + // thing that distinguishes them, which is the point. + expect(props["canary_reason_test_beta"]).toBe("forced_on"); + }); + + it("reports CI as excluded rather than out_of_cohort", () => { + systemState.is_ci = true; + // Both are enabled:false, but `excluded` was never bucketed. Conflating + // them is what biased the first fleet accuracy read low. + expect(canaryEventProperties()["canary_reason_test_gamma"]).toBe("excluded"); + }); + + it("reports telemetry_opt_out when the preference is off", () => { + configState.telemetryEnabled = false; + // Emitted for completeness; by construction such an install sends nothing, + // so this value should never actually be observed in the warehouse. + expect(canaryEventProperties()["canary_reason_test_alpha"]).toBe("telemetry_opt_out"); + }); +}); diff --git a/packages/cli/src/telemetry/canary.ts b/packages/cli/src/telemetry/canary.ts index f14de2e5b..f42aa612b 100644 --- a/packages/cli/src/telemetry/canary.ts +++ b/packages/cli/src/telemetry/canary.ts @@ -180,13 +180,22 @@ export function canaryDecisionsForStudio(): Record { /** * Canary assignments as PostHog flag properties — `$feature/canary-` - * set to `"true"` / `"false"` for every registered canary. Spread onto every - * event so any metric can be broken down by cohort using PostHog's native - * flag tooling, with nothing configured server-side. See - * `canaryFeatureProperties` for why non-enrolled canaries are emitted too. + * set to `"true"` / `"false"` for every registered canary, each paired with a + * `canary_reason_`. Spread onto every event so any metric can be broken + * down by cohort using PostHog's native flag tooling, with nothing configured + * server-side. See `canaryFeatureProperties` for why non-enrolled canaries are + * emitted too, and why the reason has to ride along. + * + * The reason is what makes a cohort flip attributable. `resolveCanary` has + * computed it all along and this function used to drop it, so an install + * reporting both values looked identical whether a developer had toggled + * `HF_CANARY_*` or the bucketer had genuinely disagreed with itself. */ export function canaryEventProperties(): Record { return canaryFeatureProperties( - CANARIES.map((c) => ({ name: c.name, enabled: resolveCanary(c.name).enabled })), + CANARIES.map((c) => { + const { enabled, reason } = resolveCanary(c.name); + return { name: c.name, enabled, reason }; + }), ); } diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index 19ef6a71e..aeb87d710 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -1,7 +1,12 @@ import { describe, expect, it } from "vitest"; import { canaryBucket, evaluateCanary, parseCanaryOverride, type CanaryInput } from "./canary.js"; import { CANARIES, canaryEnvVar, findCanary, overdueCanaries } from "./canaryRegistry.js"; -import { CANARY_FEATURE_PREFIX, canaryFeatureKey, canaryFeatureProperties } from "./canary.js"; +import { + CANARY_FEATURE_PREFIX, + canaryFeatureKey, + canaryFeatureProperties, + canaryReasonKey, +} from "./canary.js"; const base = (over: Partial = {}): CanaryInput => ({ feature: "test-feature", @@ -371,3 +376,56 @@ describe("PostHog flag-shaped properties", () => { expect(canaryFeatureProperties([])).toEqual({}); }); }); + +// The attribution property. Without it, an install reporting both "true" and +// "false" for a canary whose percentage never moved is indistinguishable from +// a developer toggling HF_CANARY_*. The first calibration read hit exactly +// that: 304 installs reported both values and the anomalous ones could not be +// separated from deliberate overrides. +describe("canary reason property", () => { + it("rides alongside the assignment, outside the $feature namespace", () => { + const props = canaryFeatureProperties([ + { name: "de-parallel-router", enabled: true, reason: "in_cohort" }, + ]); + expect(props["$feature/canary-de-parallel-router"]).toBe("true"); + expect(props["canary_reason_de_parallel_router"]).toBe("in_cohort"); + }); + + // A non-boolean under `$feature/` would corrupt the flag's own breakdowns, + // which is the whole reason the reason gets its own key. + it("never puts a reason inside the flag namespace", () => { + const props = canaryFeatureProperties([{ name: "x", enabled: false, reason: "forced_off" }]); + for (const [key, value] of Object.entries(props)) { + if (key.startsWith(CANARY_FEATURE_PREFIX)) { + expect(value).toMatch(/^(true|false)$/); + } + } + }); + + it("separates a forced override from a genuine cohort roll at the same value", () => { + const forced = canaryFeatureProperties([{ name: "f", enabled: true, reason: "forced_on" }]); + const rolled = canaryFeatureProperties([{ name: "f", enabled: true, reason: "in_cohort" }]); + // Identical assignment — only the reason tells them apart. This is the + // distinction the calibration read could not make. + expect(forced["$feature/canary-f"]).toBe(rolled["$feature/canary-f"]); + expect(forced["canary_reason_f"]).not.toBe(rolled["canary_reason_f"]); + }); + + it("emits `excluded` for CI, which replaces joining on is_ci", () => { + const props = canaryFeatureProperties([{ name: "c", enabled: false, reason: "excluded" }]); + // `excluded` and `out_of_cohort` are both enabled:false but mean different + // things — CI was never bucketed, the other lost the roll. Counting them + // together is what biased the first accuracy read low. + expect(props["canary_reason_c"]).toBe("excluded"); + }); + + it("omits the reason key when no reason is supplied", () => { + const props = canaryFeatureProperties([{ name: "n", enabled: true }]); + expect(props["$feature/canary-n"]).toBe("true"); + expect(props).not.toHaveProperty("canary_reason_n"); + }); + + it("sanitizes the name into a property-safe key", () => { + expect(canaryReasonKey("de-parallel-router")).toBe("canary_reason_de_parallel_router"); + }); +}); diff --git a/packages/core/src/canary.ts b/packages/core/src/canary.ts index 5fb5cd6f3..af2dfd841 100644 --- a/packages/core/src/canary.ts +++ b/packages/core/src/canary.ts @@ -175,6 +175,19 @@ export function canaryFeatureKey(name: string): string { return `${CANARY_FEATURE_PREFIX}${name}`; } +/** + * Companion key carrying WHY a canary resolved as it did. + * + * Deliberately outside the `$feature/` namespace: PostHog treats those as flag + * values and a non-boolean there would corrupt the flag's own breakdowns. This + * is an ordinary property that sits alongside. + * + * `de-parallel-router` → `canary_reason_de_parallel_router`. + */ +export function canaryReasonKey(name: string): string { + return `canary_reason_${name.replace(/[^A-Za-z0-9]+/g, "_")}`; +} + /** * Build the telemetry properties for a set of resolved canaries. * @@ -186,13 +199,26 @@ export function canaryFeatureKey(name: string): string { * * Values are the strings `"true"` / `"false"` to match how PostHog records * boolean flag values, so the property is directly comparable to a real flag. + * + * **The reason rides alongside when supplied**, under `canary_reason_`. + * Without it the assignment alone is ambiguous in the one case that matters: + * an install reporting both `"true"` and `"false"` for a canary whose + * percentage never moved is indistinguishable from a developer toggling + * `HF_CANARY_*`. The first calibration read hit exactly that wall — 304 + * installs reported both values and the genuinely anomalous ones could not be + * separated from deliberate overrides. The registry doc deferred this until + * the stability check came back dirty; it did. + * + * `reason` is optional so existing callers keep working; a caller that has the + * full decision should pass it. */ export function canaryFeatureProperties( - entries: ReadonlyArray<{ name: string; enabled: boolean }>, + entries: ReadonlyArray<{ name: string; enabled: boolean; reason?: CanaryReason }>, ): Record { const props: Record = {}; for (const entry of entries) { props[canaryFeatureKey(entry.name)] = entry.enabled ? "true" : "false"; + if (entry.reason !== undefined) props[canaryReasonKey(entry.name)] = entry.reason; } return props; }