mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-11 14:50:02 +00:00
feat(core,cli): emit the canary decision reason alongside the assignment
The calibration contract deferred this until the stability check came back dirty. It did: the first fleet read found 304 installs (1.08%) reporting both values for a canary whose percentage never moved, and the genuinely anomalous ones could not be separated from a developer toggling HF_CANARY_*, because the assignment alone is identical in both cases. resolveCanary has always computed the reason and canaryEventProperties dropped it. Now every canary emits canary_reason_<name> beside its assignment. Deliberately outside the $feature/ namespace: PostHog treats those as flag values, and a non-boolean there would corrupt the flag's own breakdowns. Two of the six wire values are immediately useful beyond override attribution. 'excluded' identifies CI installs, which today have to be dropped by joining on is_ci — conflating them with out_of_cohort is what made the first accuracy read look like a significant failure (9.22% against a 10% target) when it was not. 'no_unit_id' surfaces the fails-closed corner. The reason is optional on the core helper so existing callers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
fc0298de4a
commit
076657a639
@@ -142,6 +142,9 @@ describe("telemetry opt-out is canary opt-out", () => {
|
|||||||
"$feature/canary-test-alpha": "false",
|
"$feature/canary-test-alpha": "false",
|
||||||
"$feature/canary-test-gamma": "false",
|
"$feature/canary-test-gamma": "false",
|
||||||
"$feature/canary-test-beta": "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-alpha": "true",
|
||||||
"$feature/canary-test-gamma": expect.stringMatching(/^(true|false)$/),
|
"$feature/canary-test-gamma": expect.stringMatching(/^(true|false)$/),
|
||||||
"$feature/canary-test-beta": "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();
|
__resetCanaryCacheForTests();
|
||||||
@@ -235,3 +241,36 @@ describe("CLI canary binding", () => {
|
|||||||
expect(canaryEventProperties()["$feature/canary-test-alpha"]).toBe("false");
|
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");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -180,13 +180,22 @@ export function canaryDecisionsForStudio(): Record<string, CliCanaryDecision> {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Canary assignments as PostHog flag properties — `$feature/canary-<name>`
|
* Canary assignments as PostHog flag properties — `$feature/canary-<name>`
|
||||||
* set to `"true"` / `"false"` for every registered canary. Spread onto every
|
* set to `"true"` / `"false"` for every registered canary, each paired with a
|
||||||
* event so any metric can be broken down by cohort using PostHog's native
|
* `canary_reason_<name>`. Spread onto every event so any metric can be broken
|
||||||
* flag tooling, with nothing configured server-side. See
|
* down by cohort using PostHog's native flag tooling, with nothing configured
|
||||||
* `canaryFeatureProperties` for why non-enrolled canaries are emitted too.
|
* 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> {
|
export function canaryEventProperties(): Record<string, string> {
|
||||||
return canaryFeatureProperties(
|
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 };
|
||||||
|
}),
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,7 +1,12 @@
|
|||||||
import { describe, expect, it } from "vitest";
|
import { describe, expect, it } from "vitest";
|
||||||
import { canaryBucket, evaluateCanary, parseCanaryOverride, type CanaryInput } from "./canary.js";
|
import { canaryBucket, evaluateCanary, parseCanaryOverride, type CanaryInput } from "./canary.js";
|
||||||
import { CANARIES, canaryEnvVar, findCanary, overdueCanaries } from "./canaryRegistry.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 => ({
|
const base = (over: Partial<CanaryInput> = {}): CanaryInput => ({
|
||||||
feature: "test-feature",
|
feature: "test-feature",
|
||||||
@@ -371,3 +376,56 @@ describe("PostHog flag-shaped properties", () => {
|
|||||||
expect(canaryFeatureProperties([])).toEqual({});
|
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");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -175,6 +175,19 @@ export function canaryFeatureKey(name: string): string {
|
|||||||
return `${CANARY_FEATURE_PREFIX}${name}`;
|
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.
|
* 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
|
* Values are the strings `"true"` / `"false"` to match how PostHog records
|
||||||
* boolean flag values, so the property is directly comparable to a real flag.
|
* 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(
|
export function canaryFeatureProperties(
|
||||||
entries: ReadonlyArray<{ name: string; enabled: boolean }>,
|
entries: ReadonlyArray<{ name: string; enabled: boolean; reason?: CanaryReason }>,
|
||||||
): Record<string, string> {
|
): Record<string, string> {
|
||||||
const props: Record<string, string> = {};
|
const props: Record<string, string> = {};
|
||||||
for (const entry of entries) {
|
for (const entry of entries) {
|
||||||
props[canaryFeatureKey(entry.name)] = entry.enabled ? "true" : "false";
|
props[canaryFeatureKey(entry.name)] = entry.enabled ? "true" : "false";
|
||||||
|
if (entry.reason !== undefined) props[canaryReasonKey(entry.name)] = entry.reason;
|
||||||
}
|
}
|
||||||
return props;
|
return props;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user