You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
19 near-identical public wrappers.telemetry.rs has 17 track_* functions and 2 spawn_* functions (telemetry.rs#L442-L889). Each one differs only in its event type, its hard-coded command string and a json! metadata literal, and each ends with api_token, org_slug.
A new HTTP client for every event.send_telemetry_event calls reqwest::Client::builder()…build() for every event it sends (telemetry.rs#L293-L310). That is a new TLS context and connection pool every time; it is one of the 6 production Client::builder() sites (C15).
Credentials are threaded through every command by hand. There are ~45 call sites in 10 command files: remove 11, rollback 7, vendor 7, get 5, and apply, repair, scan, vex 3 each, plus scan vendor_flow and list. Each passes api_token.as_deref(), org_slug.as_deref(), and 5 CLI helper signatures exist only to carry them (remove.rs 4, get.rs 1).
Credentials are resolved two different ways at 10 sites.
list and vex use GlobalArgs::telemetry_credentials() (args.rs#L484-L496).
apply, get, scan, vendor and repair read the getters of their API client.
remove and rollback build a client just for telemetry (remove.rs#L330-L333, rollback.rs#L1096-L1099).``
A client-derived slug can come from org auto-resolve, which telemetry_credentials() deliberately skips. So whether an event goes to /v0/orgs/<slug>/telemetry or to the public proxy depends on which command sent it.
Correction to the review: "125 signatures" overstates the threading. On 045d7ec it is about 45 call sites and 5 helper signatures, plus the 19 wrapper signatures in core.
Impact
This is maintenance cost, not a live bug. Every new event or field means another wrapper and more threading. The two credential strategies mean a reader can't tell which endpoint an event reaches without tracing the command. Building a client per event costs a TLS setup for each send on the critical path of apply and remove.
Proposed change
In core, add pub enum TelemetryEvent { Applied { count, dry_run }, ApplyFailed { error, dry_run }, … }, one variant per current wrapper. Each variant has a command() and a metadata() method holding today's literals unchanged.
Add pub struct Telemetry { token, org, client: OnceLock<reqwest::Client>, pending: PendingTelemetry } with track(&self, TelemetryEvent) (inline) and spawn(&mut self, TelemetryEvent) / flush() (background). The client is built once, with today's 2 s connect and 5 s total timeouts.
In the CLI, build one Telemetry per command at the point that today resolves credentials, keeping each command's current strategy so this PR changes no behavior, and pass &Telemetry instead of (api_token, org_slug).
Delete: the 19 track_*/spawn_* wrappers, fire, fire_prepared, the per-send Client::builder(), and the token/org parameters on the 5 CLI helpers.
crates/socket-patch-core/src/telemetry.rs (about −350 / +150 production lines) and ~45 one-line call-site edits across 10 CLI command files. Estimated under 600 changed production lines. Mechanical: the event bodies stay byte-identical.
Acceptance criteria
Every event body is byte-identical to today's: event type, context.command, metadata keys and values, error shape. Extend inline_and_background_scan_trackers_post_the_same_event into a table over every TelemetryEvent variant that compares against a golden JSON.
One reqwest::Client per process for telemetry. Add a test that sends two events through one Telemetry and checks that they share the client (for example, a build counter behind cfg(test)).
is_telemetry_disabled, --no-telemetry, --offline and the flush ordering before stdout keep their current behavior; the args.rs telemetry-gate tests stay green.
No api_token: Option<&str> / org_slug: Option<&str> parameter remains in crates/socket-patch-cli/src just for telemetry.
cargo test -p socket-patch-core telemetry, cargo test -p socket-patch-cli and cargo clippy --workspace --all-features -- -D warnings stay green.
[agent] Filed by the scheduled architecture audit routine (CLI and core). Register: discussion #560 register.
Kind: refactor. Source: review 7.5 / R15; register C22.
Problem
Verified on main @
045d7ec.19 near-identical public wrappers.
telemetry.rshas 17track_*functions and 2spawn_*functions (telemetry.rs#L442-L889). Each one differs only in its event type, its hard-coded command string and ajson!metadata literal, and each ends withapi_token, org_slug.A new HTTP client for every event.
send_telemetry_eventcallsreqwest::Client::builder()…build()for every event it sends (telemetry.rs#L293-L310). That is a new TLS context and connection pool every time; it is one of the 6 productionClient::builder()sites (C15).Credentials are threaded through every command by hand. There are ~45 call sites in 10 command files: remove 11, rollback 7, vendor 7, get 5, and apply, repair, scan, vex 3 each, plus scan
vendor_flowand list. Each passesapi_token.as_deref(), org_slug.as_deref(), and 5 CLI helper signatures exist only to carry them (remove.rs4,get.rs1).Credentials are resolved two different ways at 10 sites.
listandvexuseGlobalArgs::telemetry_credentials()(args.rs#L484-L496).apply,get,scan,vendorandrepairread the getters of their API client.removeandrollbackbuild a client just for telemetry (remove.rs#L330-L333,rollback.rs#L1096-L1099).``A client-derived slug can come from org auto-resolve, which
telemetry_credentials()deliberately skips. So whether an event goes to/v0/orgs/<slug>/telemetryor to the public proxy depends on which command sent it.Correction to the review: "125 signatures" overstates the threading. On
045d7ecit is about 45 call sites and 5 helper signatures, plus the 19 wrapper signatures in core.Impact
This is maintenance cost, not a live bug. Every new event or field means another wrapper and more threading. The two credential strategies mean a reader can't tell which endpoint an event reaches without tracing the command. Building a client per event costs a TLS setup for each send on the critical path of
applyandremove.Proposed change
pub enum TelemetryEvent { Applied { count, dry_run }, ApplyFailed { error, dry_run }, … }, one variant per current wrapper. Each variant has acommand()and ametadata()method holding today's literals unchanged.pub struct Telemetry { token, org, client: OnceLock<reqwest::Client>, pending: PendingTelemetry }withtrack(&self, TelemetryEvent)(inline) andspawn(&mut self, TelemetryEvent)/flush()(background). The client is built once, with today's 2 s connect and 5 s total timeouts.Telemetryper command at the point that today resolves credentials, keeping each command's current strategy so this PR changes no behavior, and pass&Telemetryinstead of(api_token, org_slug).track_*/spawn_*wrappers,fire,fire_prepared, the per-sendClient::builder(), and the token/org parameters on the 5 CLI helpers.RunCtx(C10); the new handle makes it a one-line change later.Size and scope
crates/socket-patch-core/src/telemetry.rs(about −350 / +150 production lines) and ~45 one-line call-site edits across 10 CLI command files. Estimated under 600 changed production lines. Mechanical: the event bodies stay byte-identical.Acceptance criteria
context.command, metadata keys and values, error shape. Extendinline_and_background_scan_trackers_post_the_same_eventinto a table over everyTelemetryEventvariant that compares against a golden JSON.reqwest::Clientper process for telemetry. Add a test that sends two events through oneTelemetryand checks that they share the client (for example, a build counter behindcfg(test)).is_telemetry_disabled,--no-telemetry,--offlineand theflushordering before stdout keep their current behavior; theargs.rstelemetry-gate tests stay green.api_token: Option<&str>/org_slug: Option<&str>parameter remains incrates/socket-patch-cli/srcjust for telemetry.cargo test -p socket-patch-core telemetry,cargo test -p socket-patch-cliandcargo clippy --workspace --all-features -- -D warningsstay green.Dependencies
RunCtxwould own theTelemetry) and Decide: where patch API calls go when a token is set but the org slug can't be resolved #648 (a single place to choose the endpoint).