Skip to content

Send telemetry through one Telemetry handle with a shared HTTP client instead of 19 track wrappers #770

Description

[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.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.
  • Out of scope: unifying the two credential strategies, which changes which endpoint some events reach. That belongs with Decide: where patch API calls go when a token is set but the org slug can't be resolved #648 (C07) and 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

  • 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.

Dependencies

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent:triagedarch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)priority:p3refactorStructural change: duplicated code or logic, missing abstraction, layering, dead code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions