Skip to content

fix(cli): add missing cache fields to telemetry test fixture - #1915

Merged
vanceingalls merged 1 commit into
mainfrom
07-03-fix_cli_add_missing_cache_fields_to_telemetry_test_fixture
Aug 21, 2026
Merged

fix(cli): add missing cache fields to telemetry test fixture#1915
vanceingalls merged 1 commit into
mainfrom
07-03-fix_cli_add_missing_cache_fields_to_telemetry_test_fixture

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

What

Brief description of the change.

Why

Why is this change needed?

How

How was this implemented? Any notable design decisions?

Test plan

How was this tested?

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (if applicable)

ExtractionPhaseBreakdown gained cachePublishFailures/cacheGcEvictions/
cacheGcBytesFreed/cacheAgedPartialsCleared; the studioRenderTelemetry
test fixture was never updated, breaking Typecheck on main and every PR
based on it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Closing in the backlog value sweep. This is documentation, test-only, dependency upkeep, refactoring, or narrow diagnostic/ergonomic polish without enough current user impact or an active merge path to justify carrying it open. Please reopen or resubmit against current main if the need is still concrete.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

R1 adversarial review — COMMENT

Delta scope: +4 fields on the videoExtractBreakdown fixture in packages/cli/src/server/studioRenderTelemetry.test.ts:112-115.

Verified

  • ExtractionPhaseBreakdown in packages/engine/src/services/videoFrameExtractor.ts:240-246 declares cachePublishFailures, cacheGcEvictions, cacheGcBytesFreed, cacheAgedPartialsCleared as required numbers. The fixture was type-unsound without them; this PR restores the fixture to match the interface. Sibling fixtures already carry the same fields (packages/producer/src/services/render/stages/extractVideosStage.test.ts, extractVideosStage.timelineBound.test.ts, videoFrameExtractor.ts:1454-1459 producer default), so the shape is consistent tree-wide.
  • Consumer under test: packages/cli/src/server/studioRenderTelemetry.ts extractPayload() reads only resolveMs / hdrProbeMs / hdr…Count / vfr… / extractMs / cacheHits / cacheMisses — the 4 new fields are not consumed. Using 0 is a safe neutral value; no render_complete assertion needs to change.
  • CI: all required checks green on head (Test, Typecheck, Lint, CLI smoke, regression shards 1-8, Windows render).

Non-blockers (observations, not asks)

  1. studioRenderTelemetry.ts:65-75 (extractPayload) and packages/cli/src/telemetry/events.ts:268-277 (RenderCompleteProps.extract*) both stop at extractCacheMisses. The 4 new phase-breakdown counters (cachePublishFailures, cacheGcEvictions, cacheGcBytesFreed, cacheAgedPartialsCleared) are collected by the extractor and now live on the fixture but are dropped on the floor before hitting PostHog. The comment on cachePublishFailures in videoFrameExtractor.ts:237-239 explicitly calls it "the first signal that warm renders are silently going cold" — worth a follow-up PR that extends extractPayload + adds matching extract_cache_publish_failures / extract_cache_gc_* / extract_cache_aged_partials_cleared keys to EventMap and the trackRenderComplete mapper. Out of scope here; flagging so it doesn't get lost.
  2. The mapping test at studioRenderTelemetry.test.ts:170-190 currently only asserts on the two extract-cache fields the consumer already emits (extractCacheHits, extractCacheMisses); when the follow-up above lands, this test is the right home for the four new field assertions and will need to be extended in the same PR to keep mutation-escapability tight.

Verdict: LGTM as a minimal fixture-completeness fix. No blockers.

— Via

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve. Independent read. Four new cache-telemetry fields added to the RenderPerfSummary fixture with safe zero defaults. Consumer only reads pre-existing fields, so the addition is backward-compatible. PostHog extraction is a follow-up concern (Via's note). Minimal change, no risk.

— Miga

@vanceingalls
vanceingalls merged commit 24b3ebd into main Aug 21, 2026
94 checks passed
@vanceingalls
vanceingalls deleted the 07-03-fix_cli_add_missing_cache_fields_to_telemetry_test_fixture branch August 21, 2026 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants