Give each install an anonymous identity - #822
Merged
Merged
Conversation
Events could not become people. Nothing identified an install — no distinct_id, no device id, nothing persisted — so 400 pushes could have been 4 users or 40, and retention and funnels were unanswerable. This is the question quiltsync-telemetry-host closed on as "blocked on whether one may be attached at all"; the answer is that counting installs needs nothing about a user. A random UUID, minted on first run and persisted beside the app's own data, derived from nothing. Not an email hash: that is reversible for a customer set already known, so it would carry the obligations of personal data while feeling as though it did not — and it counts accounts, not installs, being absent before the first login, which is exactly where the onboarding funnel starts. Reaches all three sinks so they can be read together: the analytics distinct_id, the crash reporter's user.id, and a field in the diagnostic export. A crash now leads to that install's event stream leads to the archive its user emailed in. Two design points worth keeping: - The identity rides the crash client's *event hook*, beside the host tag, not a scope. It is fixed for the process, so this is not about staleness — a scope carrying it would still reach only threads that snapshotted after it was set. One mechanism, both facts. - `wire_payload` is separate from `event_payload` so the two stay honest about whose fact each property is: the payload belongs to the *event* and is pinned by its own tests, while distinct_id belongs to the *install* and to nothing the vocabulary describes. A payloadless event still gets a properties object, because it still has an identity — and app launch, the head of every funnel, is exactly that case. Failure semantics, which are the whole reason this is not three lines: - Cannot persist -> report no identity, never an unpersisted one. An id that is not on disk is a *new* id next launch, inflating the install count precisely when disks are unhappy. - Read fails for any reason other than absence -> report none, and do not mint a replacement, which would discard a real install's history to satisfy one run. - Written atomically (temp + rename), so an interrupted write cannot leave a truncated value that reads as a different install forever. - No identity means an *unattributed* event, never a dropped one — anything else loses the events of the machines having trouble. Locally verifiable via the dry run, which shows the identity — the first unit the rig from #820 actually pays for. just lint 0, cargo fmt --check 0, 300 quilt-sync tests, workspace green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
install-id -> install_id. Every other file in app_local_data_dir is snake_case (publish_settings.json, autosync_settings.json, fswatcher_settings.json); the kebab spelling was an inconsistency, not a choice. Recorded what is convention and what is departure, so neither reads as an oversight: one concern with one FILE_NAME resolved against app_local_data_dir is exactly what the settings modules already do, and there is no shared settings framework to join instead. It departs in two ways on purpose — a bare value rather than JSON, because `cat install_id` answers the question a crash report raises and wrapping one opaque string buys nothing; and an atomic write, because a torn settings file regenerates from defaults while a torn identity silently becomes a different install forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`path_in` was a free function of my own invention. The persisted-settings modules here all do the same thing already: a private `Self::file_path(data_dir)` associated function over a module-level FILE_NAME, with load/save on the type. Adopted that, so identity reads like its siblings. Following the convention exposed a real defect. Every one of those modules calls `create_dir_all` before writing and mine did not — and identity is loaded at main.rs:80, *before* `init_file_logging` at :91, which is the only thing that brings the data directory into being. So on a genuinely fresh install the write failed, `load` returned None, and the first session of every install went unattributed, with the identity appearing only from the second launch. That is precisely the launch every funnel starts from, so the bug would have quietly undercut the metric this unit exists to enable. Fixed by `create_dir_all` in `save`, and pinned by a test that loads from a data directory that does not exist yet — asserting both that an id appears and that the next launch reports the same one. Kept as a departure, now stated next to the convention it departs from: `load` returns Option where the settings return Result-with-defaults, because identity has no meaningful default and a fabricated one is worse than none. just lint 0, cargo fmt --check 0, 301 quilt-sync tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The diagnostics page enumerates what a crash report sends, and that list no longer matched what it sends. A disclosure that lags the data collection by a release is the wrong way round, so it ships with the thing it discloses rather than waiting for a docs sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two review catches. **The metadata comment was false.** It claimed a missing `install_id` meant "no identity" and never "an older export" — but `skip_serializing_if` omits the key, so those two cases produced byte-identical JSON and the field answered neither question. Now always written, `null` when there is no identity, so the two are different bytes; `serde(default)` is what keeps the older shape parsing. This is the rule the event vocabulary already follows — an unattributed event reports a null host rather than dropping the property, because a reader cannot divide by a value that was never written. Pinned by a test asserting both halves. **`as_str().to_string()` read like a double conversion.** It is one allocation — `as_str()` is a free reborrow and `InstallId` wraps a String we only ever hold by reference — but it hid a real inefficiency in the crash hook, which rebuilt the whole `User` on *every* event despite the identity being fixed for the process, which is what the comment right above it already said. Built once at config time and cloned per send instead, so the code says what the comment claims. `to_owned` elsewhere, which states "an owned copy" rather than implying formatting. just lint 0, cargo fmt --check 0, 302 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
@greptileai please re-review |
This was referenced Aug 5, 2026
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Second code unit of quiltdata/quilt-specs#37 (
w-install-id), after #820. Decisions and rationale live there; the design is #install-identity.Problem
Events could not become people. Nothing identified an install — no
distinct_id, no device id, nothing persisted — so 400 pushes might be 4 users or 40, and retention and funnels were unanswerable. This is the question the previous telemetry change closed on as "blocked on whether one may be attached at all". The answer turns out to be narrow: counting installs needs nothing about a user.Approach
A random UUID, minted on first run and persisted beside the app's own data, derived from nothing.
Not an email hash, which fails twice over: it is reversible for a customer set already known, so it would carry the obligations of personal data while feeling as though it did not — and it counts accounts, being absent before the first login, which is exactly where the onboarding funnel starts.
Three questions, three identifiers, none of them personal: how many people is this id; which customer is the catalog host, already collected; which account, if ever needed, is an opaque id from the registry, never derived on the client.
It reaches all three sinks so they can be read together — Mixpanel
distinct_id, Sentryuser.id, and a field in the diagnostic export. A crash now leads to that install's event stream leads to the archive its user emailed in.Two design points worth review
The identity rides the crash client's event hook, beside the host tag — not a scope. Not because it changes, but because a scope carrying it would still reach only the threads that snapshotted after it was set. That is the same thread-local-hub trap the host rule already documents; one mechanism, both facts.
wire_payloadis separate fromevent_payloadso the two stay honest about whose fact each property is. The payload belongs to the event and is pinned by its own tests;distinct_idbelongs to the install and to nothing the vocabulary describes. A payloadless event still gets a properties object, because it still has an identity — andapp_launched, the head of every funnel, is exactly that case.Failure semantics are most of the work
A first-run bug this caught
Review asked why I had invented a
path_inhelper when the persisted-settings modules already share a shape. Adopting theirs (Self::file_path+create_dir_allbefore write) exposed a real defect: the identity is loaded atmain.rs:80, beforeinit_file_loggingat:91— which is the only thing that brings the data directory into being. Withoutcreate_dir_all, a genuinely fresh install failed to persist and its first session went unattributed, with the id appearing only from the second launch. That is the one launch every funnel starts from.Fixed, and pinned by a test that loads from a directory that does not exist yet, asserting both that an id appears and that the next launch reports the same one.
One departure from those siblings is kept and stated beside the convention:
loadreturnsOptionwhere they returnResult-with-defaults, because identity has no meaningful default and a fabricated one is worse than none.Also here
The diagnostics page enumerates what a crash report sends, and that list no longer matched. Flagging it explicitly since it is scope beyond the unit — a disclosure that lags the data collection by a release seemed the wrong way round, so it ships with the thing it discloses instead of waiting for the docs sweep. Easy to drop if you would rather it went with the rest of the user-facing wording.
Verification
just lintexit 0,cargo fmt --checkexit 0, 301 quilt-sync tests, workspace green.Eleven new tests: eight on the identity itself (mint-and-persist, stability across loads, distinct per install, fresh-install-with-no-data-dir, whitespace trimming, empty-file replacement, opaque values honoured verbatim, unpersistable →
None) and four on the wire form (identity on a payloadless event, does not displace the event payload, absent identity leaves the event unchanged, dry run shows it).Locally checkable — this is the first unit the dry-run rig from #820 pays for:
🤖 Generated with Claude Code
Greptile Summary
The PR creates and persists an anonymous per-install UUID, then consistently attaches it to analytics, crash reports, and diagnostic exports.
distinct_idto Mixpanel payloads anduser.idto Sentry events.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains within the eligible follow-up-review scope.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Startup["Application startup"] --> Load["Load or atomically persist install_id"] Load --> Telemetry["Telemetry state"] Telemetry --> Mixpanel["Mixpanel distinct_id"] Telemetry --> Sentry["Sentry user.id"] Telemetry --> Diagnostics["Diagnostic metadata.json"]Reviews (2): Last reviewed commit: "Write an absent identity as null, and bu..." | Re-trigger Greptile
Context used: