Repository navigation
Send one cli.command telemetry event per run - #46
Conversation
bed039d to
01569df
Compare
bc5fadf to
9380184
Compare
9380184 to
d96df00
Compare
d96df00 to
316be25
Compare
316be25 to
8a1a4b0
Compare
8a1a4b0 to
a1edc04
Compare
a1edc04 to
b1ad124
Compare
11b71a2 to
b28e7d5
Compare
aba0649 to
df533ea
Compare
df533ea to
25b4cda
Compare
25b4cda to
dca2b1a
Compare
18a5f2c to
ebf2948
Compare
Each run builds one cli.command event from the run record, under the privacy rules reviewed in mapbox/event-schema#300: a value only for flags, fixed choices, numbers and language, country or feature-type codes; coordinates and tile addresses by name only; free strings as lengths; --data by the keys its spec declares; a userId replaced daily; no request ids. A detached child sends it to production Mapbox Events, so the command never waits. cli_token::send tries tokens in the order its caller declares: the user's --token or MAPBOX_ACCESS_TOKEN, then the login (refreshed only when about to expire), then the CLI's own token (MAPBOX_CLI_TOKEN, then a pk. token compiled in from MAPBOX_CLI_BUNDLED_TOKEN; build.rs refuses anything else). With no token the event is dropped. MAPBOX_CLI_NO_TELEMETRY=1 turns it off, and runs under sudo record nothing. MAPBOX_INTERNAL_TELEMETRY_URL redirects the send in builds that are not a production release, and MAPBOX_INTERNAL_TELEMETRY_LOG logs each delivery. On Windows, detached children no longer hold the caller's stdout pipe open.
The Privacy section still said only the service is collected and never credentials. It now lists the command and the shape of its options, the token kind and account, and that Mapbox Events keeps the token an event is sent with. The API commands section lists the event's fields.
- Send a language or country value only when it is exactly a code, so a short word can't pass as one. - Read `--data @<path>` again only when it is a regular file, so a FIFO can't hang the exit. - Count `MapboxAccessToken` as the user's token, and honor `--use-login` on runs that never parse. - Skip the event for `uninstall` on Windows, where the sender would keep `mapbox.exe` locked. - Make `auth logout` wait for a refresh in flight so it can't write the login back. - Keep `cargo test` from posting to production Events, and test the sudo skip, `--q`'s privacy and new numeric location parameters. - List every field the event carries in README's Privacy section.
c3a1d3b to
b733f7a
Compare
mattpodwysocki
left a comment
There was a problem hiding this comment.
Went deep on this one given what it's doing, first time this CLI sends anything recurring rather than a one-time install ping. Read telemetry_event.rs directly rather than trusting the description: coordinates really do send name only (classify() returns early for anything in COORDINATES, backed by every_numeric_location_parameter_is_a_coordinate which walks every bundled spec and fails the build if a future lon/lat-shaped parameter isn't added to that list), free strings really do reduce to length only, and language/country/types only send the actual value when it's strictly code-shaped. the_event_sends_only_fields_the_schema_declares diffing against a hand-maintained field list is exactly the kind of test that catches a field silently added later without a schema/legal update, and no_request_id_is_sent closes the one leak I'd have worried about most.
Confirmed it's genuinely fire-and-forget: the exit code is computed before run_record::finish() runs, and the actual POST happens in a detached child (reusing update_check's own detach() primitive), with a direct test that sends against a loopback server that never responds and confirms the command doesn't wait. Liked that MAPBOX_INTERNAL_TELEMETRY_URL's production gate is a compile-time const fn checked independently in both the parent and the detached child, so a compromised parent can't hand the child an override a release build would honor. Token fallback order matches the description exactly, and it travels over the child's stdin rather than argv or env, with the user's token explicitly removed from the child's environment before spawning.
Build, fmt, clippy, full suite (unit + the loopback integration tests) all pass. This is some of the most carefully verified privacy-surface code I've seen in this repo, nice work. Approving.
Stacked on #91.
Each run sends one
cli.commandevent to Mapbox Events from a detached child, so the command never waits. The schema and Legal's review are in mapbox/event-schema#300.language/country/types; coordinates send their name, other strings their length. README's Privacy section lists every field.pk.token as a last fallback (cli_token.rs). A 401 tries the next one.MAPBOX_INTERNAL_TELEMETRY_URLoverrides it only in non-release builds.MAPBOX_CLI_NO_TELEMETRY=1,completion,sudo, anduninstallon Windows.Verified: tests against a loopback server; staging events reached the warehouse. Not verified: Windows locally, and the release pipeline's bundled token (mapbox-cli-private).
Before release: mapbox/event-schema#300 merges and
MAPBOX_CLI_BUNDLED_TOKENis set in mapbox-cli-private.