feat(telemetry): notify users of telemetry collection on first run - #10
feat(telemetry): notify users of telemetry collection on first run#10Hweinstock wants to merge 4 commits into
Conversation
Hweinstock
left a comment
There was a problem hiding this comment.
lets simplify this approach. Lets just remove the logic that globalConfigAccessor populates the installationId as a side effect, then ecplicitly at the root level check,
does installationId exist in global config?
- if no, set it to a random uuid, and set isFirstRun (a local variable) to true.
- if yes, do nothing.
This will require updating the default installationId to undefined.
| exit_reason: "success", | ||
| }); | ||
| // Capture first-run state up front: routing and telemetry emission both read |
There was a problem hiding this comment.
remove these useless comments. Also the isFirstRun should still return true, even if the installationId was written as a side effect.
| /** | ||
| * In-memory GlobalConfigAccessor for tests. | ||
| * In-memory GlobalConfigAccessor for tests. Represents an already-installed |
There was a problem hiding this comment.
no need to change this comment.
| } | ||
| /** Writes the telemetry notice to the given stream, but only on a first run. */ | ||
| export function printFirstRunNotice(isFirstRun: boolean, out: { write(text: string): void }): void { |
There was a problem hiding this comment.
lets just inline the function above here, and only have one function.
| @@ -0,0 +1,34 @@ | |||
| import { test, describe, expect } from "bun:test"; | |||
There was a problem hiding this comment.
we only need one test, a test.each that checks verifies isFirstRun prints, and isFirstRun false doesn't.
| set(newConfig: GlobalConfig): Promise<GlobalConfig>; | ||
| /** | ||
| * Returns true on the first run of the CLI, i.e. when no installationId has | ||
| * yet been persisted to the config file. Reflects the state before {@link get} |
There was a problem hiding this comment.
just replace this with Returns true on the first run of the CLI on a new machine.
| public async isFirstRun(): Promise<boolean> { | ||
| if (this.firstRun !== undefined) return this.firstRun; | ||
| // A first run is one where no installationId has been persisted yet. A |
| @@ -0,0 +1,69 @@ | |||
| import { test, describe, beforeEach, afterEach, expect } from "bun:test"; | |||
| const configFileData = await this.readConfigFile(); | ||
| // capture first-run state before we populate an installationId below, so a |
| /** | ||
| * Default values for the global config. Includes a unique installationId for each process. | ||
| * Default values for the global config. installationId is unset by default and |
There was a problem hiding this comment.
remove the installationId part of this comment.
| try { | ||
| await globalConfigAccessor.set({ ...globalConfig, installationId: crypto.randomUUID() }); | ||
| } catch { | ||
| // best effort |
There was a problem hiding this comment.
we should log the error here, using the pattern below.
1e8a58f to
cfb9da2Comparecfb9da2 to
8254946Compare
Summary
Notify users of telemetry collection (and how to opt out) on the first run of the CLI — when no
installationIdhas yet been persisted to~/.agentcore/config.json. The notice is printed to stderr and never shown again once aninstallationIdexists.Spec
Problem: The user is never notified of telemetry collection, or of ways to opt out.
Definition of Done:
installationIdin~/.agentcore/config.json), print a notice that the CLI collects usage analytics, mirroring the message style ofsrc/cli/notices.tsonmain.installationIdalready exists.Base branch:
refactor.Implementation
First-run detection is done explicitly at the root; the config accessor no longer mutates config as a side effect.
installationIddefaults to unset.DEFAULT_GLOBAL_CONFIG.installationIdisundefinedandGlobalConfig.installationIdis optional.DefaultGlobalConfigAccessor.get()no longer generates or persists an id.src/index.ts):const isFirstRun = config.installationId === undefined. On first run, generatecrypto.randomUUID()and persist it viaglobalConfigAccessor.set(...)(best-effort) before routing — telemetry validatesinstallation_idas a required UUID, so a value must exist by the time metrics emit. The notice is then printed in thefinallyblock.src/telemetry/notice.ts: a singleprintFirstRunNotice(isFirstRun, out).Fallout from making the default id unset
config.test.tsxtests that asserted the previous accessor auto-population — that behavior is intentionally relocated to the root (still exercised end-to-end below).installationId(they model an already-installed CLI, whose resource attributes require a UUID).Verification
Automated
bun test— 2557 pass, 0 fail.bun run typecheck— clean.bun run lint:check— clean.Manual (end-to-end, isolated
HOME)Notice count on stderr — Run 1:
1, Run 2:0.How to reproduce