Skip to content

feat(media-use): classify and tag heygen provider telemetry, harden test isolation#2130

Open
miguel-heygen wants to merge 5 commits into
mainfrom
feat/heygen-conversion-telemetry
Open

feat(media-use): classify and tag heygen provider telemetry, harden test isolation#2130
miguel-heygen wants to merge 5 commits into
mainfrom
feat/heygen-conversion-telemetry

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

media-use's heygen-cli integration could tell you a resolve succeeded, but not why it failed along the way, or whether a successful call went through the free (OAuth) or paid (API-key) path. This closes both gaps, plus a real test-isolation bug where local test runs could reach the production telemetry endpoint.

Companion change to heygen-com/heygen-cli#212 (identity unification + auth telemetry) — the two land independently since neither repo's release blocks the other.

What changed

  • Failure classification: heygen-cli errors (not found, not authenticated, outdated, rate limited) now map to a small stable reason instead of a raw, unbounded error string — including a new rate-limit/quota case that wasn't recognized before.
  • Free vs. paid tagging: successful heygen-backed resolves now record whether the call used an OAuth session or an API key, without changing the existing auth-header logic other call sites depend on.
  • Test isolation fix: local test runs previously relied solely on an env-var default to avoid hitting the real telemetry endpoint. That's now backed by a real interception seam (verified with a local test server), so the safety doesn't depend on every future test remembering to set the env var.

Design decisions

  • The new failure reason is a stable reason value, not the display message or raw stderr text — stays a small enum a dashboard can group by, not free text.
  • The free/paid signal is a new property, not encoded into the existing provider string, so it doesn't touch any dashboard tile that already keys off provider name.

Testing

  • Full skills suite: 203 passed, 1 failed (pre-existing, unrelated to this change).
  • oxlint / oxfmt --check: clean across all changed files.

Post-Deploy Monitoring & Validation

  • No runtime/production monitoring needed beyond normal telemetry review — this is additive, best-effort instrumentation with no behavior change to resolve outcomes.
  • Watch for: no increase in resolve failures or latency (telemetry calls are fire-and-forget and never block a resolve).

Adds a stable reason-code classifier (not_found/not_authenticated/
outdated/rate_limited/other) alongside the existing display-message
classifier, and reports it via a new media_use_provider_error event so
quota/rate-limit failures are distinguishable from other provider
errors even when a fallback provider later succeeds.
Adds a sparse auth_method property (oauth/api_key) to media_use_resolve
telemetry for heygen-family providers, derived via a new heygenAuthMethod()
helper alongside the existing heygenAuthHeaders(). Absent for every
non-heygen provider.
…ception seam

resolve.test.mjs already set DO_NOT_TRACK=1 for every spawned resolve.mjs
invocation, so local test runs were not actually leaking events. But that
safety relied entirely on every call site remembering to set the env var,
with no way to prove it. Add a MEDIA_USE_TELEMETRY_HOST override read at the
point telemetry.mjs builds its POST url (falling back to the real endpoint
when unset), and a test that points it at a local HTTP server to assert a
real event is actually intercepted rather than trusting the env-var default
alone.
@miguel-heygen miguel-heygen force-pushed the feat/heygen-conversion-telemetry branch from f5d6abf to e2ed3c6 Compare July 10, 2026 16:55
Warn once on stderr if MEDIA_USE_TELEMETRY_HOST is ever set outside a
test/CI context, since the existing catch{} around the network call
would otherwise swallow a silent redirect with zero signal. Also widen
the rate_limited classifier to catch "too many requests" (the literal
HTTP 429 reason phrase) and "throttled", which were falling through to
"other".
reportHeygenFailure's telemetry call was fire-and-forget with no join
point, and its callers (voice-provider.mjs, heygen-search.mjs) are sync
call sites that can't await it themselves. On the resolve-miss path,
process.exit() could follow shortly after, racing the still-in-flight
request on its own non-keepalive connection and dropping the event.

Track each pending report in a module-level set and flush it before the
miss/success telemetry a process.exit() may follow.

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the diff end-to-end (all 9 files), traced the telemetry send path + flush ordering, and checked the classifier for exhaustiveness. Code LGTM — no correctness blockers. Clean and well-tested, and the telemetry-privacy invariant holds: reason is a fixed enum (never raw stderr), auth_method is oauth/api_key/absent, and undefined values are dropped by JSON.stringify. The isolation fix genuinely closes the env-var-default gap with a real interception seam.

⚠️ Merge blocker (edited in after cross-checking with @tai): CI is red — the Skills: manifest in sync job is failing on a stale skills-manifest.json hash (all other 30+ checks pass). Regenerate the manifest (bun run --cwd packages/cli gen:skills-manifest) and commit before merge. Code is clean; this is a stale generated-artifact gate, not a logic issue.

Also worth addressing — email-case identity split: heygenAccountDistinctId (telemetry.mjs) uses .trim() only and the heygen-cli side uses the raw Email — neither lowercases. If backend/downstream ingestion case-normalizes emails inconsistently, an uppercase-email user splits into two PostHog profiles, undercounting the exact conversion this stack measures. Consider .trim().toLowerCase() on both sides, or confirm downstream already normalizes.

Non-blocking nits:

  1. resolve.mjs:420-424 — in the provenance literal, ...heygenAuthMethodFor(provider) is spread before ...searchResult.metadata?.provenance. Inert today (no provider emits authMethod in provenance), but a future one would shadow the computed value. Since the computed value is the intended source of truth, consider spreading it after the provenance (or documenting the precedence).
  2. telemetry.mjsisTestOrCiContext() (keys on NODE_ENV==="test") and optedOut() (keys on "development") are near-identical env predicates serving different purposes. Intentional, but a cross-referencing comment or shared helper would prevent future drift. No bug — the warning path is only reachable after optedOut() returns false (posthogHost() is only called from postBatchtrack() past its guard).
  3. heygen.mjsheygenAuthMethod() and heygenAuthHeaders() independently encode "Authorization header ⇒ OAuth"; a shared isOauthCred() helper would single-source that invariant.
  4. heygen-cli.mjs rate-limit classifier — lower.includes("quota")/"throttled" are broad substring matches on combined stderr+stdout (e.g. "quota check passed" would misclassify as rate_limited). Acceptable since reason is a coarse grouping enum, not behavior-gating — just noting the inherent precision ceiling of substring matching.

Questions:

  1. Cache-hit auth_method semantics: on a cache/manifest hit, result() emits the auth method stored when the asset was first fetched; on a fresh resolve it's the current method. So the same asset resolved under OAuth then re-resolved from cache under an API key reports oauth both times. Is "how the asset was originally obtained" the intended meaning of the tag? (I think that's the right choice for a free-vs-paid conversion signal — worth a one-line comment since the two paths compute it differently.)
  2. reused-explicit path (resolve.mjs:1062) preserves the pre-existing authMethod from the cached record — confirming that's intended, consistent with Q1.

— Somu

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.

2 participants