feat(auth): identify accounts and add structured login telemetry#212
feat(auth): identify accounts and add structured login telemetry#212miguel-heygen wants to merge 6 commits into
Conversation
…nd structured login events Switch the analytics client's distinct id to the same cross-tool install identity already shared by the other HeyGen CLIs (promoting any existing local id on first run so upgraders keep their identity), and point events at the same ingestion project so a person's activity is queryable in one place instead of two. Add a `surface` property to distinguish this CLI's traffic there, plus IdentifyAccount/AuthLoginStarted/AuthLoginCompleted/ AuthLoginFailed methods so a login attempt becomes structured, queryable telemetry (method + specific failure reason) instead of a generic event.
Link a successful login (OAuth or API-key) to the account identity via PostHog identify, disclose telemetry + identity linking on first run, and emit structured started/completed/failed events per login attempt with method and failure reason so login funnels are queryable by branch instead of a generic command-complete event.
…vent runAPIKeyLogin and runOAuthLogin had several early-return error branches (malformed credentials file, save/clear failures, PKCE/state generation, loopback startup, authorize-URL build, response encoding) that returned without emitting AuthLoginCompleted or AuthLoginFailed, leaving their AuthLoginStarted event dangling with no outcome. Rather than instrumenting each scattered return site, wrap both runners with a deferred catch-all: a per-call `reported` flag is set at each existing AuthLoginCompleted/AuthLoginFailed call, and a defer fires AuthLoginFailed(method, "internal_error") only when the function returns an error and nothing else already reported an outcome. New error paths added later are covered automatically instead of silently reintroducing the gap.
1d0a531 to
857dbd3
Compare
runOAuthLogin and runAPIKeyLogin marked the login as completed and fired AuthLoginCompleted before writing the success response via ctx.formatter.Data. If that write failed (e.g. a broken pipe on stdout), the command still exited non-zero but telemetry had already recorded a completed login. Reorder so completion is only reported once the response write actually succeeds; a write failure now falls through to the existing deferred reconciliation, which reports AuthLoginFailed instead.
somanshreddy
left a comment
There was a problem hiding this comment.
Reviewed the full diff (all 8 files, ~1324 lines), traced the auth + telemetry flow end-to-end, and cross-checked against the hyperframes scheme + the posthog-go SDK. LGTM — no blocking issues. The fire-and-forget contract holds and login behavior isn't altered. I specifically checked the classic login-telemetry leak vectors:
- No secret/token/PII in event properties —
AUTH_LOGIN_*carry onlymethod+ a fixedreasonenum plus base props (command,cli_version,os,arch,surface,client_origin,$ip: null). No API key, OAuth token, or PKCE verifier. - The email/username is sent, but only as the PostHog
distinct_idinIdentifyAccount— the intended, disclosed cross-tool account identity that matches hyperframes'email || username. By-design identity, not a leak.
On the Alias{DistinctId: account, Alias: anon} + Identify{DistinctId: account} pair vs hyperframes' single $identify + $anon_distinct_id: verified against posthog-go v1.11.2 (alias.go / identify.go / posthog.go) — Identify has no anon field and Alias serializes to $create_alias, so the two produce an equivalent bidirectional merge. Your reasoning is correct. The only residual difference: heygen-cli also emits a bare Identify (no $set props) alongside the alias — a harmless extra event.
Non-blocking nits:
internal/analytics/analytics.go—IdentifyAccountmutatesc.identifiedwithout synchronization. No real race today (login is single-goroutine), but a one-line comment noting that assumption (like the one onStarted()) would harden intent if analytics is ever invoked from the loopback/browser goroutines.main.go:98-103—maybeShowTelemetryNoticewrites its owntelemetry_notice_shownflag in heygen-cli's config dir rather than the sharedtelemetryNoticeShownkey. Consequence: a user who already saw the notice via media-use sees it again from heygen-cli (and vice versa). Product decision, not a bug — flagging so it's a conscious choice.auth_login.godevice-code path firesAUTH_LOGIN_STARTED+AUTH_LOGIN_FAILEDfor the unsupported flag on every invocation. Fine — just confirm you want device-code attempts counted in the funnel (probably yes; it measures demand for the feature).
Questions:
- Was the
Alias+Identifypair validated against the actual PostHog project to confirm the person-merge stitches identically in dashboards? The SDK reasoning checks out statically; a one-time live confirmation would fully close it. - Your PR notes
go testwasn't runnable in your env (no Go toolchain) — can we get a CI-green confirmation? Correctness here rests on reading rather than execution, and the test coverage of the new paths is genuinely strong, so a green run is the last mile.
— Somu
…join An uppercase-cased email or username would otherwise split one HeyGen account across two PostHog profiles, since hyperframes-oss's side of the identity join already lowercases.
os.UserHomeDir reads USERPROFILE on Windows, not HOME, so these tests leaked the real CI runner's home directory on windows-latest.
Summary
heygen-cli's own analytics reported to a separate PostHog project under a locally-minted identity, with no identify call on login — so a successful
heygen auth logincouldn't be joined to anything the rest of the toolchain already tracks, and login attempts only ever showed up as a generic command-run event with no method or failure reason. This wires heygen-cli into the same cross-tool identity and destination, and makes login itself first-class, queryable telemetry.Companion change to a matching PR in
heygen-com/hyperframes: heygen-com/hyperframes#2130 (media-use side: failure classification + free/paid tagging + test-isolation hardening). The two land independently — neither repo's release blocks the other.What changed
surfaceproperty so a query can still isolate this CLI's traffic.auth loginattempts now emit distinct started/completed/failed events carrying the method (OAuth vs. API-key) and, on failure, a specific reason drawn from the login flow's real failure branches (timeout, cancellation, invalid input, etc.) instead of a generic pass/fail.startedevent with no matchingcompleted/failed; every path now reconciles through a safety-net terminal event.Design decisions
Testing
go testrun; verified by careful manual reading and cross-checking against the pinned SDK's actual source.Post-Deploy Monitoring & Validation