mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-07 18:26:17 +00:00
fix(producer): guard pmset behind shouldTrack + don't pin workers without streaming (review)
Two review findings on the floor/telemetry PR: 1. powerStateFields() is spread into the properties object at the CALL SITE, so it ran before trackEvent's own `if (!shouldTrack()) return` guard — telemetry-disabled installs paid two blocking `pmset` subprocess spawns per render for an event that was then discarded. Now short-circuits on shouldTrack() (memoized, so no cost on the tracked path). Regression test asserts pmset is not sampled when telemetry is off; fault-injection verified it fails without the guard. 2. The DE parallel router pinned workerCount to 3 and skipped calibration even when verified parallel DE STREAMING — the entire reason for the pin — could not run for that render. The common case is a composition over streamingEncodeMaxDurationSeconds (240 s default): the duration cap disables streaming before the router's force flag is consulted, so the render got a hard-coded 3 workers chosen by a benchmark for a path it was not on, instead of the calibrated count. shouldPreferParallelDrawElement now takes parallelStreamingAvailable and withholds the bet without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3da31e399b
commit
b99c803898
@@ -2,9 +2,20 @@ import { describe, expect, it, vi, beforeEach } from "vitest";
|
||||
|
||||
const trackEvent = vi.fn();
|
||||
const flush = vi.fn(() => Promise.resolve());
|
||||
const shouldTrack = vi.fn(() => true);
|
||||
vi.mock("./client.js", () => ({
|
||||
trackEvent: (...args: unknown[]) => trackEvent(...args),
|
||||
flush: () => flush(),
|
||||
shouldTrack: () => shouldTrack(),
|
||||
}));
|
||||
|
||||
// Power state shells out to `pmset`; spy so tests can assert it is NOT
|
||||
// sampled for opted-out installs (the fields are built at the call site,
|
||||
// before trackEvent's own shouldTrack guard).
|
||||
const getPowerState = vi.fn(() => ({ on_battery: true, low_power_mode: false }));
|
||||
vi.mock("./system.js", async () => ({
|
||||
...(await vi.importActual<typeof import("./system.js")>("./system.js")),
|
||||
getPowerState: () => getPowerState(),
|
||||
}));
|
||||
|
||||
// identifyUser reads the install anonymousId; pin it so the $identify alias is
|
||||
@@ -653,3 +664,28 @@ describe("auth login telemetry events", () => {
|
||||
expect(trackEvent).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("power-state sampling respects the telemetry opt-out", () => {
|
||||
beforeEach(() => {
|
||||
getPowerState.mockClear();
|
||||
shouldTrack.mockReturnValue(true);
|
||||
});
|
||||
|
||||
it("samples power state for a tracked render", () => {
|
||||
trackRenderComplete({ durationMs: 1, fps: 30, quality: "high", docker: false });
|
||||
expect(getPowerState).toHaveBeenCalled();
|
||||
const props = trackEvent.mock.calls.at(-1)?.[1] as Record<string, unknown>;
|
||||
expect(props.on_battery).toBe(true);
|
||||
expect(props.low_power_mode).toBe(false);
|
||||
});
|
||||
|
||||
it("does NOT spawn pmset when telemetry is disabled", () => {
|
||||
// Regression: powerStateFields() is spread into the properties object at
|
||||
// the call site, so it runs BEFORE trackEvent's `if (!shouldTrack())`
|
||||
// guard — an opted-out install would otherwise pay two blocking
|
||||
// subprocess spawns per render for an event that is then discarded.
|
||||
shouldTrack.mockReturnValue(false);
|
||||
trackRenderComplete({ durationMs: 1, fps: 30, quality: "high", docker: false });
|
||||
expect(getPowerState).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { redactTelemetryString, type OutputResolutionIssueKind } from "@hyperframes/core";
|
||||
import type { SubTimelineWaitOutcome } from "@hyperframes/engine";
|
||||
import { FEEDBACK_RATING_SCALE } from "../utils/feedbackRating.js";
|
||||
import { flush, trackEvent } from "./client.js";
|
||||
import { flush, shouldTrack, trackEvent } from "./client.js";
|
||||
import { readConfig } from "./config.js";
|
||||
import { getPowerState } from "./system.js";
|
||||
|
||||
@@ -10,7 +10,15 @@ import { getPowerState } from "./system.js";
|
||||
// render_complete AND render_error: the DE fleet is macOS laptops whose
|
||||
// power management shifts render perf ~1.8x with no other telemetry signal,
|
||||
// and perf/soak analysis needs to segment by it (see getPowerState).
|
||||
//
|
||||
// shouldTrack() is checked HERE, not just inside trackEvent: this helper is
|
||||
// spread into the properties object at the CALL SITE, so it runs before
|
||||
// trackEvent's own `if (!shouldTrack()) return` guard. Without this an
|
||||
// opted-out install would still pay two blocking `pmset` subprocess spawns
|
||||
// per render for an event that is then discarded (review finding).
|
||||
// shouldTrack() memoizes, so this costs nothing on the tracked path.
|
||||
function powerStateFields(): { on_battery?: boolean; low_power_mode?: boolean } {
|
||||
if (!shouldTrack()) return {};
|
||||
const power = getPowerState();
|
||||
return {
|
||||
on_battery: power.on_battery ?? undefined,
|
||||
|
||||
Reference in New Issue
Block a user