perf(telemetry): resolve the machine ID once per process, not once per event - #2101
Open
Soph wants to merge 2 commits into
Open
perf(telemetry): resolve the machine ID once per process, not once per event#2101Soph wants to merge 2 commits into
Soph wants to merge 2 commits into
Conversation
…r event
machineid.ProtectedID is not a cheap lookup. On macOS it shells out to
`ioreg -rd1 -c IOPlatformExpertDevice`; measured p50 11.8ms. Nothing cached
it, and every payload builder calls it — including BuildSkillEventPayload,
which TrackSkillInvocationsDetached runs once per skill event.
So the per-event cost was a subprocess, paid synchronously in the parent hook
process. Measured on the pre-fix code (spawn hooked out, so this is parent-side
blocking time only):
1 event 11.5 ms
5 events 55.9 ms
20 events 217.6 ms
Linear at ~11.6ms/event. #2023 batched the detached sends to remove per-event
process cost, but those spawns were non-blocking; the blocking `ioreg` calls it
left behind were the dominant term. Removing the 10-event cap in the same change
— correct for data fidelity — also removed the accidental ~115ms bound on this.
The machine ID cannot change while the process runs, so resolve it once with
sync.OnceValues. A 20-event batch goes from 217.6ms to one resolve plus ~60us of
payload building, and the cost stops scaling with event count. Every other event
type shares the cache, so a hook that emits skill events and a commit-condensed
event pays one lookup between them, and every `entire` command drops ~11.6ms
from BuildEventPayload.
paths.WorktreeRoot memoizes `git rev-parse` for the same reason; this follows
that precedent rather than inventing a pattern.
The error is cached too. A failed lookup means callers skip telemetry (nil
payload), and retrying per event would reintroduce the subprocess this removes.
Tests pin the contract that matters — one platform lookup per process across 20
payload builds and across event types, cached failure, and a single resolve under
32 concurrent first callers. Verified all three fail against the unfixed
behavior (21, 5, and 32 lookups respectively).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces telemetry overhead by memoizing the app-scoped machine ID (machineid.ProtectedID("entire-cli")) for the lifetime of the process, avoiding repeated expensive platform lookups (notably the macOS ioreg subprocess) during batched telemetry payload construction.
Changes:
- Added a process-lifetime cached machine ID accessor (
telemetryMachineID) backed bysync.OnceValues, caching both success and failure. - Updated all telemetry payload builders to use the cached machine ID instead of calling
machineid.ProtectedIDper event. - Added unit tests covering once-per-process behavior, cached failures, and concurrent first-callers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/entire/cli/telemetry/machine_id.go | Introduces telemetryMachineID() with sync.OnceValues to cache machine ID resolution for the process lifetime. |
| cmd/entire/cli/telemetry/machine_id_test.go | Adds tests validating caching semantics and concurrency behavior for the machine ID lookup. |
| cmd/entire/cli/telemetry/detached.go | Switches command/plugin/skill telemetry payload builders to use telemetryMachineID() instead of per-event machine ID lookups. |
| cmd/entire/cli/telemetry/checkpoint_policy.go | Switches checkpoint-policy telemetry payload builder to use telemetryMachineID(). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review feedback on #2101 read withMachineIDResolver's global swap as a data race against the parallel payload-builder tests in this package. It is not one: Go resumes parallel top-level tests only after every sequential one has finished, so the swap window and those tests never overlap. Probed it directly — a 150ms sequential window holding the globals open, three parallel tests sampling an in-flight flag: zero overlaps, and no parallel test had even started when the last sequential test ended. `-race -count=5` over the whole package is clean. But the requirement was only a comment, so the next person to add t.Parallel() to one of these tests would have turned it into a real race silently. t.Setenv makes Go enforce it: adding t.Parallel() to a caller now panics with "test using t.Setenv or t.Chdir can not use t.Parallel" instead of racing. Verified by adding it temporarily. Chosen over making the cache instance-based, which would reshape production code for test convenience.
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.
https://entire.io/gh/entireio/cli/trails/1125
machineid.ProtectedIDis not a cheap lookup. On macOS it shells out toioreg -rd1 -c IOPlatformExpertDevice(measured p50 11.8 ms); Linux reads a file, Windows the registry. Nothing cached it, and every payload builder calls it — includingBuildSkillEventPayload, whichTrackSkillInvocationsDetachedruns once per skill event.So the per-event cost was a subprocess, paid synchronously in the parent hook process. Measured on the pre-fix code with the detached spawn hooked out, so this is parent-side blocking time only:
Linear at ~11.6 ms/event.
#2023 batched the detached sends specifically to remove per-event process cost — but those spawns were non-blocking, and the blocking
ioregcalls it left behind were the dominant term. Removing the 10-event cap in the same change (correct for data fidelity) also removed the accidental ~115 ms bound on this, and the uncapped path is exactly the one that drains a session's whole skill-event backlog in one call.The fix
Resolve it once per process with
sync.OnceValues. The machine ID cannot change while the process runs.A 20-event batch goes from 217.6 ms to one resolve plus ~60 µs of payload building, and the cost stops scaling with event count. All four builders share the cache, so a hook that emits skill events and (with #2024) a commit-condensed event pays one lookup between them — and every
entirecommand drops ~11.6 ms fromBuildEventPayload.paths.WorktreeRootmemoizesgit rev-parsefor the same reason; this follows that precedent rather than inventing a pattern.The error is cached too, deliberately: a failed lookup means callers skip telemetry (nil payload), and retrying per event would reintroduce the subprocess this removes.
Deliberately not done
Two further steps, both out of scope here:
gitVersion— that would take it to zero on the hook path, butDistinctIDis set by the parent today, andSendEventsis lenient precisely because a self-update can hand a payload to a different build. An older child receiving an emptyDistinctIDwould send events with none.Tests
The contract that matters, each verified to fail against the unfixed behavior:
mise run fmt,mise run lintclean. 9,645 unit / 529 integration / canary 56+4 all green.Note
Low Risk
Internal telemetry memoization only; distinct ID derivation and skip-on-failure behavior are unchanged.
Overview
Stops resolving
machineid.ProtectedIDon every telemetry payload. That lookup shells out toioregon macOS (~12ms) and was running once per skill event, so a 20-event batch blocked ~218ms on the parent hook path.All payload builders now share
telemetryMachineID(), which usessync.OnceValuesfor process-lifetime caching (success and failure). Tests cover once-per-process across event types, cached failures dropping payloads, and concurrent first callers.Reviewed by Cursor Bugbot for commit 7d57754. Configure here.