Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 134
feat(onboarding): first-run funnel telemetry#1049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
dfccc03
feat(onboarding): telemetry taxonomy + first batch of funnel events
sahrizvi a0989d8
fix(onboarding): telemetry review fixes — shutdown races, gateway gaps
sahrizvi 0bad651
feat(onboarding): activation funnel events + first_prompt_sent
sahrizvi 77f5635
fix(onboarding): activation-funnel review fixes — scoping, attributio…
sahrizvi bd41e2a
feat(onboarding): TUI funnel events via an injected telemetry callback
sahrizvi de8c30f
fix(onboarding): TUI funnel review fixes — abandonment scope, guards,…
sahrizvi 24f7b77
fix(onboarding): add launch correlation id + full-review fixes
sahrizvi cb3b8e8
fix(onboarding): don't nest altimate_change markers inside the TUI cl…
sahrizvi 58828b5
test(onboarding): funnel emission coverage
sahrizvi e2b37c1
test(onboarding): cover the first-run provider picker
sahrizvi 1b16a15
fix(onboarding): actually share launch_id between the TUI and worker …
sahrizvi 134b80c
fix(onboarding): attribute tool-emitted funnel events to their own se…
sahrizvi b6d7cc2
fix(onboarding): drive the scan gate from setup completion, not readi…
sahrizvi 66b0777
fix(onboarding): consensus review fixes for #1049
sahrizvi 51936dc
fix(onboarding): record the provider chosen after "Search all provide…
sahrizvi 1c21544
fix(onboarding): third-round consensus review — 9 majors, 11 minors
sahrizvi fe6ac08
fix(onboarding): address bot review findings on #1049
sahrizvi a949fdc
fix(onboarding): address coderabbit review findings on #1049
sahrizvi 3875d00
fix(onboarding): repair regressions the first round of bot fixes intr…
sahrizvi 6828ef1
fix(onboarding): scope gateway events, guard the catalogue, fix Marke…
sahrizvi b66bbe9
fix(onboarding): un-nest a marker pair that exposed an upstream produ…
sahrizvi 8e6d52f
fix(onboarding): close an unterminated marker block, de-order the lau…
sahrizvi 9976bfc
docs(onboarding): state the exit-flush bound and the bias it puts on …
sahrizvi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -3,6 +3,28 @@ import { createServer } from "http" | ||
| import { randomBytes } from "crypto" | ||
| import open from "open" | ||
| import { AltimateApi } from "../api/client" | ||
| // altimate_change — onboarding telemetry for the gateway sign-in funnel | ||
| import * as OnboardingTelemetry from "../telemetry/onboarding" | ||
| /** | ||
| * Why a failure reason is attached at the rejection site rather than inferred from the message: | ||
| * `gateway_auth_failed.reason` is a closed enum, and the callback's catch sees only an Error. | ||
| * Matching on message text would silently drift the moment any of these strings is reworded, and | ||
| * the `error` query param is attacker-influenced text we must not parse or forward. Tagging the | ||
| * error where the cause is known keeps the classification deterministic — and note that an | ||
| * unknown/invalid `state` never rejects a pending flow at all (the handler 400s without touching | ||
| * the map), so a CSRF mismatch legitimately surfaces later as `timeout`, not `denied`. | ||
| */ | ||
| type GatewayFailureReason = "timeout" | "denied" | "error" | ||
| function markReason(err: Error, reason: GatewayFailureReason): Error { | ||
| return Object.assign(err, { altimateGatewayReason: reason }) | ||
| } | ||
| function reasonOf(err: unknown): GatewayFailureReason { | ||
| const tagged = (err as { altimateGatewayReason?: GatewayFailureReason } | undefined)?.altimateGatewayReason | ||
| return tagged ?? "error" | ||
| } | ||
| // Loopback port range the CLI listens on for the browser to deliver the gateway | ||
| // credential after sign-in. We prefer 7317 (mnemonic + otherwise unused in this | ||
| @@ -153,7 +175,8 @@ async function doStartCallbackServer(): Promise<void> { | ||
| const error = url.searchParams.get("error") | ||
| if (error) { | ||
| entry.reject(new Error(error)) | ||
| // altimate_change — the browser reported an explicit failure: the only true `denied` signal | ||
| entry.reject(markReason(new Error(error), "denied")) | ||
| html(200, HTML_ERROR(error)) | ||
| return | ||
| } | ||
| @@ -257,7 +280,8 @@ function stopCallbackServer() { | ||
| function registerPending(state: string, timeoutMs = 15 * 60 * 1000): Promise<CallbackResult> { | ||
| return new Promise<CallbackResult>((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| if (pending.delete(state)) reject(new Error("Timed out waiting for browser sign-in")) | ||
| // altimate_change — tag as `timeout` for gateway_auth_failed classification | ||
| if (pending.delete(state)) reject(markReason(new Error("Timed out waiting for browser sign-in"), "timeout")) | ||
| // If the dialog was dismissed and callback() never ran its finally, the | ||
| // loopback server would otherwise stay bound past the timeout. Free the | ||
| // port once nothing is waiting on it. | ||
| @@ -286,7 +310,19 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> { | ||
| label: "Altimate LLM Gateway", | ||
| async authorize() { | ||
| const state = randomBytes(16).toString("hex") | ||
| await startCallbackServer() | ||
| // altimate_change start — the attempt starts here, before the callback server and the | ||
| // browser open. startCallbackServer() throws when port 7317 is taken, which is a real | ||
| // and reasonably common gateway-auth failure that happens before any callback object | ||
| // exists — without this catch it would never appear in the funnel. | ||
| const startedAt = Date.now() | ||
| try { | ||
| await startCallbackServer() | ||
| } catch (err) { | ||
| if (OnboardingTelemetry.isFunnelActive()) | ||
| void OnboardingTelemetry.emit({ type: "gateway_auth_failed", reason: "error" }) | ||
| throw err | ||
| } | ||
| // altimate_change end | ||
| // Register the pending flow BEFORE opening the browser so an instant | ||
| // redirect can be matched by state rather than dropped as CSRF. | ||
| const result = registerPending(state) | ||
| @@ -321,6 +357,27 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> { | ||
| // Removed to stop leaking state-bearing authorize URLs into logs. | ||
| await open(authorizeUrl).catch(() => undefined) | ||
| // altimate_change start — onboarding funnel: gateway sign-in started. | ||
| // Spec name is `gateway_device_code_issued`; this flow is a browser loopback OAuth | ||
| // with no device code, so the event means "authorize URL built, browser open | ||
| // attempted". open() failures are swallowed above (the URL is also printed for the | ||
| // user to paste), so this fires even when no browser actually launched. | ||
| // The URL is never sent — it carries the CSRF `state`. | ||
| if (OnboardingTelemetry.isFunnelActive()) void OnboardingTelemetry.emit({ type: "gateway_device_code_issued" }) | ||
| // One outcome per attempt. callback() closes over `result` and re-runs its whole body | ||
| // on every invocation, so a repeated call would otherwise re-emit completion/failure | ||
| // (and re-report a connect time measured from the original attempt). | ||
| // | ||
| // Tri-state rather than a boolean because concurrent invocations race for it and a | ||
| // plain latch let the WRONG one win: two callbacks exchanging the same one-time token | ||
| // means one fails fast, claims the latch, and reports gateway_auth_failed while the | ||
| // other goes on to save valid credentials — the user is connected and the funnel says | ||
| // they are not. Success is authoritative, so it emits even after a failure was | ||
| // reported; failure only reports when nothing else has. | ||
| let outcome: "none" | "failed" | "completed" = "none" | ||
| // altimate_change end | ||
| return { | ||
| url: authorizeUrl, | ||
| instructions: "Complete sign-in in your browser to connect Altimate LLM Gateway.", | ||
| @@ -344,11 +401,33 @@ export async function AltimateAuthPlugin(_input: PluginInput): Promise<Hooks> { | ||
| altimateInstanceName: creds.instance, | ||
| altimateApiKey: authToken, | ||
| }) | ||
| // altimate_change start — onboarding funnel: auth succeeded and the instance | ||
| // is live. The instance name is the customer's tenant identifier and is never | ||
| // sent. time_to_connect_ms runs from the start of authorize() — before the | ||
| // callback server and browser open, both of which are part of the wait the user | ||
| // actually experiences — and lives in this attempt's closure, so a concurrent | ||
| // attempt cannot overwrite it. | ||
| if (outcome !== "completed" && OnboardingTelemetry.isFunnelActive()) { | ||
| outcome = "completed" | ||
| void OnboardingTelemetry.emit({ type: "gateway_auth_completed" }) | ||
| void OnboardingTelemetry.emit({ | ||
| type: "instance_connected", | ||
| time_to_connect_ms: Date.now() - startedAt, | ||
| }) | ||
| } | ||
| // altimate_change end | ||
| return { type: "success", key: authToken, provider: "altimate-backend" } | ||
| } catch (err) { | ||
| // Log the reason (CSRF / timeout / invalid instance / …). Runs in the | ||
| // server process, so this goes to the log, not the TUI display. | ||
| console.error("[altimate] gateway sign-in failed:", err instanceof Error ? err.message : err) | ||
| // altimate_change — onboarding funnel: only the classified enum is sent. The | ||
| // message can embed the instance name (see the invalid-instance throw above), | ||
| // so it never reaches telemetry. | ||
| if (outcome === "none" && OnboardingTelemetry.isFunnelActive()) { | ||
| outcome = "failed" | ||
| void OnboardingTelemetry.emit({ type: "gateway_auth_failed", reason: reasonOf(err) }) | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return { type: "failed" } | ||
| } finally { | ||
| // Keep the shared server up while another flow is still waiting. | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Fast first-run gateway sign-ins can be missing from the funnel because this worker-side guard runs before the unawaited
onboardingStartedRPC has setfunnelStarted; the subsequent callback can also complete before activation and lose both outcome events. Synchronizing the worker activation (or buffering gateway events until its acknowledgement) would retain first-run telemetry without re-enabling telemetry for routine/auth.Prompt for AI agents