telemetry: link events to org/project groups; merge the pre-login anonymous id on login - #175
Conversation
… pre-login anonymous id on login and retire it on logout
There was a problem hiding this comment.
Reviewed by Yang Dong
This adds PostHog group associations and handles identity merging for direct login and logout commands. The event shapes are sound, but other supported authentication transitions bypass the new identity lifecycle, so I would request changes before merging.
Identity handling misses authentication transitions inside other commands
important · defect · correctness · src/telemetry.ts:239
This change depends on every authentication transition being an explicit login or logout command, but setup agent performs the default interactive login internally and env use permanently clears an existing session. The former never merges prior anonymous events; the latter never rotates the already-linked anonymous ID, so switching away from production and back before using the CLI logged out attributes those anonymous events to the previous account. Drive identity handling from the actual session transition, or explicitly cover these two established paths.
Evidence
read-the-code — src/telemetry.ts:229-256, src/index.ts:41-49, src/commands/setup.ts:321-326, src/commands/setup.ts:385-452, src/commands/env.ts:50-72, test/setup-agent.test.ts:135-155, test/setup-agent.test.ts:246-257. The insta and developing-insta-cli skills directed the authentication/configuration lifecycle trace.
There was a problem hiding this comment.
Reviewed by Wang Miao
This attaches org/project group keys to each CLI event and adds a $identify merge event on successful login plus an anonymous-id rotation on logout. The group guard, the login success gate and the pre-rotation capture of the id are all right, and both org and project ids are UUIDs so the ID() check on orgId passes for real values while dropping the empty INSTA_ORG_ID case. One small ordering slip is all I found, and it doesn't hold the branch.
Reading and rotating the anonymous id moved above the if (!key) return, so custom-host installs now mint a telemetry id file
minor · defect · correctness · src/telemetry.ts:238
Before this change anonymousId() was only reached after telemetryKey() returned a key, so an insta-oss or preview-host user never had ~/.insta/telemetry.json written. Now the read-or-mint (and, on logout, the rotation write) happens two lines earlier, so any command on a custom apiUrl creates and rewrites a persistent id that is never used and never sent — telemetryDisabled still short-circuits above it, so opt-out users are unaffected, and nothing leaves the machine. Moving both lines below if (!key) return restores the old behaviour without affecting the login/logout logic; the existing custom-host test only asserts calls is empty, so it doesn't notice.
Evidence
read-the-code — src/telemetry.ts:229-256 (new order), diff of the same function against 559d5621 (old call site inside the buildCommandEvent context), src/telemetry.ts:66-70 (telemetryKey returning undefined for a custom host), test/telemetry.test.ts:226-241 (the custom-host test), src/index.ts:43-49 (the guard hook that calls trackCommand for every command)
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/telemetry.ts">
<violation number="1" location="src/telemetry.ts:238">
P3: On custom hosts, this creates `~/.insta/telemetry.json` for commands that will never send telemetry. Keep logout rotation before the host check, but defer anonymous-ID lookup until after a valid telemetry key exists.</violation>
<violation number="2" location="src/telemetry.ts:253">
P2: When an explicit login target differs from an ambient `INSTA_ENV` or `INSTA_API_URL`, a successful login emits no `$identify` event. Load the persisted successful-login user for identification while continuing to suppress the previous user on failed logins.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }) | ||
| await sendBatch(key, [event], deps.fetchImpl) | ||
| const batch: object[] = [event] | ||
| if (command === 'login' && event.properties.success && config.user?.id) { |
There was a problem hiding this comment.
P2: When an explicit login target differs from an ambient INSTA_ENV or INSTA_API_URL, a successful login emits no $identify event. Load the persisted successful-login user for identification while continuing to suppress the previous user on failed logins.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/telemetry.ts, line 253:
<comment>When an explicit login target differs from an ambient `INSTA_ENV` or `INSTA_API_URL`, a successful login emits no `$identify` event. Load the persisted successful-login user for identification while continuing to suppress the previous user on failed logins.</comment>
<file context>
@@ -237,10 +245,14 @@ export async function trackCommand(cmd: Command, args: unknown[], outcome: Outco
})
- await sendBatch(key, [event], deps.fetchImpl)
+ const batch: object[] = [event]
+ if (command === 'login' && event.properties.success && config.user?.id) {
+ batch.push({ event: '$identify', distinct_id: config.user.id, timestamp: event.timestamp, properties: { $anon_distinct_id: anon } })
+ }
</file context>
There was a problem hiding this comment.
9c826ff no longer keys on the login command. The merge fires on whichever later command sees the session, so a login under a contradicting INSTA_ENV is picked up by the next command run without the override.
| let config = await (deps.loadConfig ?? readGlobal)() | ||
| const target = loginTarget(command, cmd.opts()) | ||
| if (target && normalizeUrl(target) !== normalizeUrl(config.apiUrl)) config = { apiUrl: target } | ||
| const anon = await anonymousId(deps.idFile) |
There was a problem hiding this comment.
P3: On custom hosts, this creates ~/.insta/telemetry.json for commands that will never send telemetry. Keep logout rotation before the host check, but defer anonymous-ID lookup until after a valid telemetry key exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/telemetry.ts, line 238:
<comment>On custom hosts, this creates `~/.insta/telemetry.json` for commands that will never send telemetry. Keep logout rotation before the host check, but defer anonymous-ID lookup until after a valid telemetry key exists.</comment>
<file context>
@@ -229,6 +235,8 @@ export async function trackCommand(cmd: Command, args: unknown[], outcome: Outco
let config = await (deps.loadConfig ?? readGlobal)()
const target = loginTarget(command, cmd.opts())
if (target && normalizeUrl(target) !== normalizeUrl(config.apiUrl)) config = { apiUrl: target }
+ const anon = await anonymousId(deps.idFile)
+ if (command === 'logout') await rotateAnonymousId(deps.idFile)
const key = telemetryKey(config.apiUrl)
</file context>
There was a problem hiding this comment.
Fixed in 9c826ff: the id file is read only after the telemetry key check, so custom hosts never write it; rotation now happens lazily on the next prod/staging command.
…d session, not the login/logout commands
|
Round 2 (9c826ff): the merge and the rotation are now driven by the persisted session, not by the
Wang Miao's ordering finding: the id-file access is back below Known and accepted: with |
There was a problem hiding this comment.
Reviewed by Yang Dong
This adds PostHog org/project grouping and safely persists anonymous-identification state across authentication changes. Following the insta development guidance for persisted auth and project context, I found no findings. The change is ready to merge: APPROVE.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…e one already merged
Three changes to the
cli_commandanalytics, all insrc/telemetry.ts:$groups: { project, org }so it counts toward the PostHog groups the platform creates (insta-platform#369/#371).orgis included only when the stored org id has an id's shape: anINSTA_PROJECT_ID-only CI run carriesorgId: ''and the auto-link fallback stores'local', and neither may become a group key.insta login(any mode) adds a$identifyevent to the same batch —distinct_id= the signed-in user,$anon_distinct_id= the random id in~/.insta/telemetry.json— so the commands run before signing in join the account. Failed logins send nothing extra.insta logoutreplaces the stored anonymous id, so later anonymous commands are not attributed to the account that left. Rotation happens before the host check so a logout from a custom host (insta-oss) still rotates.Tests in
test/telemetry.test.ts: groups present/absent/env-only, identify on success only, rotation after logout. README config row updated.Summary by cubic
Links
cli_commandanalytics to the PostHog org/project groups when a command runs inside a linked project, and merges the pre-login anonymous id into the signed-in account.$groupsis attached only for linked projects;orgis omitted when the org id is empty or'local'so CI-only and auto-link fallbacks don't become group keys.$identifyevent to the same batch and retries until PostHog confirms it; when a different account signs in, a fresh anonymous id is merged instead of the one already owned.logout,env use, or a switch to another account) rotates the stored anonymous id, so later anonymous commands aren't attributed to the departed account.Written for commit 6fabce6. Summary will update on new commits.