Route every request through http::send and collect each run in run_record - #57
Conversation
…cord No behavior changes. This is the collection that command history and the telemetry event are built from in the changes stacked on it, so neither has to touch a call site: - http::send is the one place a request is sent, so it can add each request (method, URL with the access token redacted, status, request id, sizes, timing) to the record. only_http_sends_requests holds it. - run_record collects what modules report — the parse, the token's source, type and account, each request, the error, stdout bytes, the auth step, the update notice — and main finishes it once on the way out, including before tilesets-cli execs and on a panic. It has no consumers yet. - cli() returns the exit code as a number, so the record can hold it.
Each run appends one line of execution metadata to ~/.mapbox/history/<UTC date>.jsonl, kept 30 days: the command path from the command tree, invocation, exit code, error code, duration, request count and the last five request ids. Never an argument value, URL, error message or account: arguments carry search terms, file paths and ids. The file is written through dated_jsonl: private, one file per UTC day, pruned to the retention window. On by default. `mapbox config set history off` turns it off for good, MAPBOX_HISTORY=0 or =1 for a session over the setting. With it off, nothing is written and no directory is created. With it on, the config directory is created when missing, so history works the same for someone who only ever set MAPBOX_ACCESS_TOKEN. Not recorded: --help, --version, completion, history itself, and runs under sudo, whose root-owned files would stop the user's own runs appending (aws/aws-cli#10031). `mapbox history list` shows the most recent runs, newest first, and `mapbox history show [id]` one run, the newest by default, found by any prefix of its id. The read-only-command tests that held ~/.mapbox untouched now check both sides: with history on only `history/` may appear, with it off nothing.
- `history list` text output gets a header row. - Tests that hold a run to leaving nothing on disk turn history off in their helper instead of looping over both states; tests/history.rs checks what history leaves, and that skipped runs create nothing. - README says turning history off keeps what was already recorded. - Drop the setting count from config.rs's module doc, which every new key would have to edit.
Moves dated_jsonl::shed here from the diagnostic-log PR so history, on by default, is bounded by size as well as by age. trim now keeps exactly the bytes shed hands it; the margin was applied twice before.
For each run command history records, `mapbox config set log on` (or MAPBOX_LOG=1) adds a line of detail to ~/.mapbox/logs/<UTC date>.jsonl, linked to the history record by its id: the command line with tokens redacted, which token was used, each request (method, redacted URL, status, request id, timing) and the error message. Off by default. Logging needs history. With history off it never runs, and `config set log on` refuses with history_required rather than store a setting that does nothing. A run history doesn't record gets no log either, since nothing could lead back to it. `mapbox history show` includes the log and says what became of it: diagnostics.status is captured, not_captured (logging was off) or unavailable (captured, since expired or evicted). The history record carries diagnosticsCaptured so the last two can be told apart. There is no separate logs command. Logs are kept up to 30 days and 100 MB in total. Past the limit the oldest go first, down to the line, and their history records stay. On every run, a day of logs whose day of history has expired or gone is deleted, logging on or off.
- History and the log each read the clock, so a run finishing across UTC midnight could put its log a day after its history, and the same run's cleanup then deleted it. Both lines now share one time and one day's file, which also drops next_date. - The log no longer repeats what the history record has (command, invocation, exitCode, durationMs, version). - scrub_tokens now finds a token that starts inside a word, as after a percent-encoded `=` (`%3Dpk.`) or in a short-flag cluster (`-ytpk.`). - tests/history.rs clears MAPBOX_LOG like the other suites. - README: a day of logs goes with its day of history, and the refusal follows the `history` setting, not MAPBOX_HISTORY.
mattpodwysocki
left a comment
mattpodwysocki
left a comment
There was a problem hiding this comment.
Checked out and built this, ran the full suite (fmt/clippy clean too), and read every changed file end to end. Two things worth your read, neither blocking.
completion's early opt-out (set_parsed setting finished = true and returning before the command/args/options get set) doesn't actually stop later writes: with_record never checks finished, so completion.rs's own add_stdout_bytes(script.len()) still lands in the record. Right now that's invisible since nothing reads a completion run's record, but the module doc reads as if completion is fully excluded ("promised to touch nothing on disk") when only the coarse fields are. Worth deciding now, before a consumer builds on the assumption that finished means untouched: either have with_record no-op once finished, or say explicitly that the flag only gates what finish() reports.
Smaller: resolves() in tilesets_cli checks is_file() on the PATH candidate but not its executable bit, so a same-named non-executable file in PATH would make it call finish(None) right before an exec that actually fails with EACCES. Real edge case, not worth blocking on.
Everything else held up well under actual testing: the panic hook's try_lock is exactly right for the self-deadlock case (this thread already holding RECORD when it panics inside with_record), the token redaction reuses executor's already-audited redacted_request_url rather than inventing new logic, and only_http_sends_requests is correctly scoped (grepped the whole src tree myself, the only remaining .send() calls are in http.rs's own tests).
Approving.
Add diagnostic logs, shown through mapbox history show
Record command history and add mapbox history
3 participants
No behavior change. Collects what each run did in one place, so command history (#56) and the telemetry event (#46) can be built from it without touching call sites. #46 is stacked on #56 to share its dated_jsonl writer.
http::sendis now the only place a request is sent, so every request is recorded: method, URL with the access token redacted, status, request id, sizes and timing. All direct.send()calls go through it, andonly_http_sends_requestskeeps it that way.run_recordcollects what modules report during a run: the command path, the given arguments and options, where the token came from (never the token itself), each request, the error, stdout bytes, the auth step and the update notice.mainfinishes it once on exit, also beforetilesets-cliexecs and on a panic.cli()returns the exit code as a number so the record can hold it.Nothing reads the record yet, so
run_recordhas#![allow(dead_code)]. What only one consumer needs, such as a run id or writing records to disk, is left to that consumer's PR.Not checked: the Windows
tilesets-cliexit path only compiles in CI.