Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions packages/cli/src/telemetry/canary.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
});
});
});
Expand Down Expand Up @@ -228,10 +231,46 @@ 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();
process.env.HF_CANARY_TEST_ALPHA = "off";
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");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The describe("reason reaches the event properties") block covers in_cohort (line 250), forced_on (260), excluded (267), telemetry_opt_out (274) — but not no_unit_id, which the PR body advertises as the second of "two values that pay for themselves immediately". resolveCanary("test-gamma") at line 217 already exercises no_unit_id at the resolver layer (when configState.bucketSeed = undefined), so the emission-layer test is one line: unset bucketSeed, unset any override, then expect(canaryEventProperties()["canary_reason_test_gamma"]).toBe("no_unit_id"). Same shape as the four existing tests — closes the "advertised in body, not asserted in tests" gap for a value the body specifically names. — Rames D Jusso

});
});
19 changes: 14 additions & 5 deletions packages/cli/src/telemetry/canary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,22 @@ export function canaryDecisionsForStudio(): Record<string, CliCanaryDecision> {

/**
* Canary assignments as PostHog flag properties — `$feature/canary-<name>`
* 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_<name>`. 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<string, string> {
return canaryFeatureProperties(
CANARIES.map((c) => ({ name: c.name, enabled: resolveCanary(c.name).enabled })),
CANARIES.map((c) => {
const { enabled, reason } = resolveCanary(c.name);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 The identical anti-pattern is still live in Studio. packages/studio/src/telemetry/canary.ts:264-266 reads:

export function canaryEventProperties(): Record<string, string> {
  return canaryFeatureProperties(
    CANARIES.map((c) => ({ name: c.name, enabled: resolveCanary(c.name).enabled })),
  );
}

That is the exact shape this PR just fixed on the CLI side. Studio events emit $feature/canary-<name> without a companion canary_reason_<name>, so:

  1. Any flip observed via a Studio-emitted event stays unattributable — override vs cohort roll blurs together in the same way it did CLI-side pre-feat(core,cli): emit the canary decision reason alongside the assignment #3074, in a surface that shares the bucket seed with the CLI when CLI-launched.
  2. The re-read query the docs(canary): record the first fleet read — check 3 is dirty #3030 doc plans ("re-reading this table" after emission ships) either filters to CLI-only sources — blind to Studio flips — or accepts partial coverage on the surface most likely to expose a shared-__HF_CLI_BUCKET_SEED-with-diverging-CLI-anonymousId pattern (the closest wire signal to the shared-distinct_id Docker hypothesis).
  3. Cross-surface canaries: de-parallel-router is CLI-only today, but the calibration canaries themselves emit on Studio events when a CLI-launched Studio inherits the seed. Asymmetric attribution here means the flip counts observed CLI-side and Studio-side won't reconcile.

Fix here — the Studio change is the same three-line shape as the one below, and the tests parallel canary.test.ts cleanly. If it's genuinely a follow-up (Studio event volume is smaller and Vance may want to isolate the release blast radius), name the follow-up in the PR body so the re-read query knows to filter Studio out and does not drop attribution silently. — Rames D Jusso

return { name: c.name, enabled, reason };
}),
);
}
60 changes: 59 additions & 1 deletion packages/core/src/canary.test.ts
Original file line number Diff line number Diff line change
@@ -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> = {}): CanaryInput => ({
feature: "test-feature",
Expand Down Expand Up @@ -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");
});
});
28 changes: 27 additions & 1 deletion packages/core/src/canary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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_<name>`.
* 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<string, string> {
const props: Record<string, string> = {};
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;
}