Conversation
Groundwork for client-side transfer performance telemetry. Nothing emits yet - the session hooks land in a following commit - so this is inert apart from the new config group. Config (xet_runtime): - New `telemetry` group: enabled, heartbeat_after, heartbeat_interval, request_timeout, final_flush_timeout, max_in_flight. - `HF_HUB_DISABLE_TELEMETRY` / `HF_HUB_OFFLINE` force it off. These cannot go in ENVIRONMENT_NAME_ALIASES, which maps names with identical polarity; these are inverted, so they are applied at the end of with_env_overrides where the opt-out unconditionally wins over HF_XET_TELEMETRY_ENABLED=1. - EnvVarGuard::unset, so gating tests are not perturbed by an exported value. Delivery (xet_client): - TelemetryEnvelope: the server's five-key contract, including its snake/camel mix (session_id, userAgent). Only the camelCase spelling is emitted; sending both is a 400. - TelemetrySink: no retry ever (a 429 is the server shedding load), a request timeout, and an in-flight cap that drops rather than queues. Serializes by hand because reqwest-middleware only exposes `json` under a feature. Every failure is swallowed at DEBUG. - TransferTelemetry: per-transfer identity and timing, built by RemoteClient from the *existing* authenticated client - a second build_auth_http_client would create a second TokenProvider and its own Hub refresh cycle. maybe_new returns None for disabled, dry-run, and non-http endpoints. - Client::transfer_telemetry() has a default None body, so the other six impls are untouched and local/memory/simulation clients are excluded. - Compiled out on wasm: XetRuntime has no spawn there. Payload (xet_data): - CommonMetrics / UploadMetrics / DownloadMetrics, from DeduplicationMetrics and GroupProgressReport, plus error_class over a closed vocabulary. - All f64s go through a finite guard: serde_json renders NaN and infinity as null, and one such document poisons an Elasticsearch field mapping. - Takes a TransferIdentity snapshot rather than &TransferTelemetry so the payload module tests without a XetContext or a live HTTP client. - Tests pin the exact key sets and each key's JSON type. Mappings are immutable once established, so a type change means per-document 500s and a reindex; these tests make that a build failure instead. Direction is deliberately not stored on TransferTelemetry. RemoteClient has no notion of it and threading one through would touch all 18 of its construction sites to benefit two; xet_data knows which kind of session it holds and supplies it when building the payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wires the plumbing to the two internal sessions. Every client surface bottoms out here, so no changes are needed in xet_pkg, hf_xet, git_xet, or xtool: the new XetSession API, the legacy data_client path that shipped huggingface_hub still calls, and the xtool/git_xet CLIs are all covered. Terminal documents: - FileUploadSession::finalize_impl now wraps finalize_inner and reports on both arms. The hook is here rather than in xet_pkg because XetUploadCommit::commit returns early on a finalize error, so hooking there would lose exactly the failures worth measuring. On the error path the dedup metrics were never moved out of the session, so they are read back rather than reported as zeros. - FileDownloadSession::finalize does the same. - Drop on both covers abort(), a cancelled task tree, and panics. This is the only coverage for XetDownloadStreamGroup, which holds a download session and never calls finalize(). Detached, since Drop is synchronous, and skipped outside a tokio runtime where there is nothing to send on. The existing `finalized` flag keeps a normal finalize from double-emitting. - Cancellation is classified as `cancelled`, not `error`, so user interrupts do not inflate failure-rate alerts. Heartbeats: - Started at session construction, but the task only emits once a transfer outlives heartbeat_after (5 min); short transfers never produce one, and nothing is spawned at all when the interval is zero. - The snapshot closure holds the session weakly. A strong reference would keep it alive and the Drop-based terminal report would never fire - there is a test pinning this. - Emitting a terminal document aborts the heartbeat, so a progress document can never arrive after the summary. dedup_snapshot and the Drop path both use try_lock rather than blocking: these run on runtime worker threads, and contention means a xorb upload is mid-write, in which case the snapshot would be incomplete anyway. FileDownloadSession needed no started_at of its own - duration comes from the telemetry's clock, set when its RemoteClient was built. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes PR 1. Simulation server: - POST /v1/telemetry on the local test server, recording each document. Readable via LocalServer::telemetry_docs() and LocalTestServer::telemetry_docs(), kept as raw serde_json::Value so tests assert on the exact wire shape - which is what the Elasticsearch mapping actually sees, not the Rust struct. Integration tests (xet_data/tests/test_transfer_telemetry.rs): - Upload and download each emit exactly one terminal document with the expected envelope and key set; upload-only keys are absent from download documents and vice versa. - A dropped session reports aborted/dropped, which is the only coverage XetDownloadStreamGroup gets. - Finalize followed by Drop emits once, not twice. - Disabled, HF_HUB_DISABLE_TELEMETRY, and dry-run each emit nothing. - Every value that crosses the wire is a non-null scalar, exercised with a zero-byte upload - the case most likely to divide by zero. These matter because Client::transfer_telemetry has a default None body: a RemoteClient override with a mistyped signature would compile and silently never be called, and no unit test would notice. Every test in that file is #[serial(env)], not just the one that sets HF_HUB_DISABLE_TELEMETRY. serial() serializes a test against other serial tests, not against the parallel ones it would otherwise poison - without this the env-mutating test made the rest of the binary fail intermittently. Sink tests against wiremock: - A telemetry endpoint that never answers does not hold a transfer past its flush budget. This is the "cannot block a transfer" regression guard. - A zero budget returns immediately, and a 429 is swallowed with exactly one request attempted - a retry would fail the expectation. Docs: - POST /v1/telemetry and the TelemetryEnvelope schema in the OpenAPI spec. The metrics vocabulary is deliberately not enumerated there; payload.rs is the source of truth and duplicating it would just create drift. - api_changes note covering the new config group, the defaulted trait method, the new Drop impls, and the rules for changing the key set. - README section on what is collected and the three ways to turn it off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…his repo This repo is public. The telemetry docs described how the receiving service indexes these documents - naming the storage technology, its field-mapping semantics, and its recovery procedure - none of which belongs here, and none of which this repo can keep correct anyway. The constraint the comments were explaining is real and worth stating, so it is now phrased in terms of the client's own contract: consumers assign each property a field type on first sight and cannot change it in place, so adding a key is safe while retyping or removing one is not. No behavior change; comments, doc strings, and the api_changes note only. Also corrects that note's forward reference - the follow-up PR ships the JSON Schema alone, having dropped the storage template for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sion Upload telemetry worked; download telemetry emitted nothing at all through the real client. Verified end-to-end against a local Hub, a local CAS server with the /v1/telemetry endpoint, and Elasticsearch: an upload produced its summary document, the matching download produced none. Four compounding causes, all fixed here. 1. Nothing called `FileDownloadSession::finalize()`. The upload path calls `upload_session.finalize()`, but `XetFileDownloadGroup::finish`/ `finish_blocking` only read `download_session.report()`, and the legacy `download_async` did not finalize either. `finalize` is the only route to `emit_download_terminal`, so every download was silent. All three now finalize, on the error path as well as the success path - a failed download is the case most worth reporting, and `?` on the bridge result skipped exactly that. 2. `finalize()` hardcoded `Ok(())` as the transfer result, so `classify` could only ever produce `outcome: ok`. The plumbing for error outcomes existed but was unreachable, which would have left download failure-rate alerting reading zero forever. Adds `finalize_with(outcome, error_class)` and `XetError::telemetry_class()`, mapping onto the same coarse class vocabulary `xet_data` already uses so both paths aggregate together. 3. The `Drop` safety net returned early unless it ran inside a tokio runtime context. The send is spawned on the `XetRuntime`'s own stored handle and never needed an ambient one, so the guard only disabled the path for embedders that release the last `Arc` from a foreign thread - precisely what the Python bindings do. Removed from both sessions. 4. `XetDownloadStreamGroup` had no completion hook at all, only `abort()`, so its entire coverage was the `Drop` path above. Gains `finish` / `finish_blocking`, exposed to Python as `finish()` plus context-manager support, so a consumed stream group reports `ok` rather than `dropped`. The existing tests passed throughout: they call `finalize()` directly and drop inside an async block, so neither reproduces the real caller's shape. `xet_pkg/tests/test_download_telemetry.rs` drives the public group API and asserts on what the server received; both new regression tests were confirmed to fail when the corresponding fix is reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two build failures, one of them mine. **wasm (regression from the previous commit).** `FileDownloadSession::finalize_with` put `crate::telemetry::Outcome` in its *signature*, but the whole telemetry module was gated to non-wasm, so `xet-data` stopped compiling for wasm32-unknown-unknown. Gating the method instead would have pushed `#[cfg]` onto every caller that merely names an outcome, so the vocabulary moves to a new always-compiled `telemetry::outcome` module: `Outcome`, `ERROR_CLASS_NONE`, `error_class`, `classify_error`, `outcome_for_class`. `emit` and `payload` stay gated, since those are what actually depend on `TransferTelemetry`. This is a pure move; `Outcome` never appeared in the serialized payload (`CommonMetrics::outcome` is a `&'static str` produced by `as_str()`), so the generated schema is unchanged and the compatibility gate in the follow-up PR still passes untouched. `XetDownloadStreamGroup::finish_blocking` is also gated to non-wasm, matching the other `_blocking` methods on that type - `bridge_sync` does not exist there. The async `finish` remains available on every target. **Windows (pre-existing, from the simulation-route commit).** The `not(unix)` `LocalTestServer` initializer was never given the `telemetry_docs` field, which is ungated on the struct, so `build_and_test-win` failed with E0063 on every push since. Both initializers now match their field sets exactly. Verified locally against what CI actually runs: both wasm crates check for wasm32-unknown-unknown, `cargo clippy -r -- -D warnings` is clean for the workspace and for hf_xet, nightly rustfmt is clean, and the full test suite passes. The Windows path could not be cross-compiled here (aws-lc-sys needs a Windows C toolchain), so it was verified by comparing each initializer's field set against the struct definition - which is exactly what E0063 checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Design overview — a reviewer's mapPosting this as a shared reference so anyone picking up the review can orient quickly. It's a big diff (~3.6k lines across 39 files), but the shape is smaller than the line count suggests: a self-contained telemetry subsystem plus the wiring needed to actually reach it. All diagrams below are just a redraw of what's in the code — no new claims. 1. Where the pieces liveThe measurement is split across two crates by what each can see: flowchart TD
subgraph entry["Client entry points (all bottom out in the two sessions)"]
A["XetUploadCommit.commit"]
C["Legacy data_client"]
B["XetFileDownloadGroup.finish"]
D["XetDownloadStreamGroup (finish or Drop)"]
end
subgraph xetdata["xet_data — owns the metric definitions"]
US["FileUploadSession"]
DS["FileDownloadSession"]
EMIT["emit.rs — bridges a finished session to the sink"]
PAY["payload.rs — the metric contract (u64 / f64 / bool / String)"]
OUT["outcome.rs — outcome and coarse error_class"]
end
subgraph xetclient["xet_client — owns identity, timing, delivery"]
TT["TransferTelemetry — one per transfer per direction"]
SINK["TelemetrySink — in-flight cap, never retries"]
ENV["TelemetryEnvelope — the wire body"]
end
CONF["xet_runtime — telemetry config group plus HF_HUB opt-outs"]
SERVER["POST /v1/telemetry — corresponding server-side endpoint"]
A --> US
C --> US
B --> DS
D --> DS
US --> EMIT
DS --> EMIT
EMIT --> PAY
EMIT --> OUT
EMIT --> TT
TT --> SINK
SINK --> ENV
ENV --> SERVER
CONF -. gates .-> TT
2. The lifecycle of one transfer's telemetrysequenceDiagram
participant Caller
participant Session as Session in xet_data
participant TT as TransferTelemetry
participant Sink as TelemetrySink
participant Server as Server-side endpoint
Note over Session,TT: one TransferTelemetry per transfer and direction
Session->>TT: start_heartbeat (weak ref so it never keeps the session alive)
Session->>TT: record_concurrency on each permit acquired
loop only after heartbeat_after elapses
TT->>Sink: heartbeat (detached and dropped if the in-flight cap is full)
Sink-->>Server: POST heartbeat
end
alt finalize path (success or error)
Session->>TT: emit_terminal (awaited up to final_flush_timeout)
TT->>Sink: submit_awaited
Sink-->>Server: POST terminal summary
else dropped without finalize
Session->>TT: emit_terminal_detached
TT->>Sink: submit_detached
Sink-->>Server: POST terminal summary (best effort)
end
Note over TT: a terminal_sent guard means exactly one terminal document per transfer
Fire-and-forget is the governing principle: never retried, a per-request timeout, an in-flight cap that drops rather than queues, and every failure swallowed at DEBUG. The one deliberate exception is the terminal document, which is awaited up to 3. How the
|
| Bucket | Files | What to look at |
|---|---|---|
| New telemetry subsystem | xet_client/.../telemetry/{mod,sink,envelope}.rs, xet_data/src/telemetry/{payload,emit,outcome,mod}.rs |
The core; payload.rs is the metric contract |
| Session hooks | file_upload_session.rs, file_download_session.rs |
terminal on finalize, Drop fallback, heartbeat start |
| Making prod paths emit | xet_pkg/{error.rs, legacy/data_client.rs, xet_session/*}, hf_xet/src/py_download_stream_group.rs |
the behavioral changes (below) |
| Config | xet_runtime/src/config/groups/telemetry.rs, utils/configuration_utils.rs |
HF_XET_TELEMETRY_* + the HF_HUB_* opt-outs |
| Test infra | simulation server records POSTs; test_transfer_telemetry.rs, test_download_telemetry.rs |
end-to-end coverage |
| Docs / generated | README.md, openapi/cas.openapi.yaml, api_changes/*, Cargo.locks |
— |
Where behavior actually changed (worth close eyes)
The measurement side alone wasn't sufficient — a few production paths had to change to reach it, and these are the non-additive parts:
- Downloads previously reported nothing through the Python bindings. Nothing called
FileDownloadSession::finalize(), soXetFileDownloadGroup::finish/finish_blockingand legacydownload_asyncnow finalize the session — on the error path as well as success. finalize_with(outcome, error_class)is new: the oldfinalize()hardcodedOk(()), so a download could only ever reportoutcome: okand failure-rate would have read zero forever.XetDownloadStreamGroup::finish+ Python context-manager support — purely additive; a group that is never finished still reports, asdropped.- The
Dropfallback no longer requires an ambient tokio runtime — it spawns onXetRuntime's stored handle, which is what the Python bindings need (they release the lastArcfrom a foreign thread).
Privacy, in one line
The client payload carries no file names, paths, hashes, repo ids, or user ids — only scalar performance metrics. Enabled by default; HF_XET_TELEMETRY_ENABLED=0, HF_HUB_DISABLE_TELEMETRY, or HF_HUB_OFFLINE turn it off, and the opt-outs win over an explicit enable. (Documents do become user-identifiable server-side, since the endpoint attaches request identity from the JWT — a property of the ingest side, not of this payload.)
I'll follow up with review comments separately.
rajatarya
left a comment
There was a problem hiding this comment.
Thanks, Sam — this is really carefully built. Splitting the subsystem by what each crate can see (metric definitions in xet_data, identity/timing/delivery in xet_client) is clean, and the fire-and-forget discipline is consistent all the way down — no retries, drop-not-queue backpressure, everything swallowed at DEBUG, and a ()-return send so a telemetry error physically cannot propagate into a transfer. The terminal_sent dedup across the finalize and Drop paths, the weak-reference heartbeat, the finite-guard on every f64 so a NaN can't poison a field type, and the key-set/type contract tests are all the right instincts. I also appreciate how directly the description owns the fact that downloads were reporting nothing before this — that's the kind of thing that's easy to leave unsaid.
I've posted a separate design-overview comment with a few diagrams so other reviewers can orient without reading all 39 files first.
Everything below is a question rather than a blocker — mostly about the interaction between the per-file download granularity and a few per-transfer scoped mechanisms:
- The awaited terminal flush is per-transfer, but downloads are per-file (inline on
file_download_session.rs). When the endpoint is degraded, each file's finalize can wait up tofinal_flush_timeout; a large snapshot with limited download concurrency could accumulate that. Wondering whether downloads want the detached path. outcome: droppedwill be the common case for successful stream-group downloads (inline onemit.rs), since existing callers won't adoptfinish(). Flagging so dashboards don't readdroppedas failure — and floating whetherDropcould inferokfrom a fully-completed progress report.- The in-flight cap is per-transfer, so aggregate telemetry request volume is unbounded (inline on
sink.rs) — relevant precisely because a snapshot download fans out into many per-file transfers at once.
None of these change the correctness of what's here; they're about how the feature behaves under the per-file download shape once it's pointed at a real endpoint. Really nice work.
|
|
||
| /// Sends the terminal document for this session. Best-effort and infallible. | ||
| #[cfg(not(target_family = "wasm"))] | ||
| async fn emit_terminal(&self, outcome: crate::telemetry::Outcome, error_class: &'static str) { |
There was a problem hiding this comment.
The awaited flush is a great call for the terminal document — a fully detached final send really does get lost when the process exits milliseconds later. But it's worth thinking about how it composes with download granularity: an upload amortizes this wait over one commit, whereas a download emits one document per file (a fresh group per hf_hub_download), so this awaited path runs once per file.
Under normal conditions that's just the round-trip and invisible. When the endpoint is degraded, though, each file's finalize() waits up to final_flush_timeout (2s default). Concurrent file downloads overlap that wait, but if download concurrency is well below the file count — a large model fetched a handful at a time — the 2s stacks across teardown batches.
Would it make sense for the download terminal to use the detached path (or a much shorter budget), and reserve the awaited flush for cases where the process is actually about to exit? The upload commit is the clean fit for awaiting; per-file downloads seem like the case where the multiplied cost outweighs the delivery gain. Curious whether you'd already weighed this.
| let Some(telemetry) = telemetry_of_download(client) else { | ||
| return; | ||
| }; | ||
| let metrics = download_metrics(&telemetry, progress, n_files, Outcome::Dropped, ERROR_CLASS_NONE); |
There was a problem hiding this comment.
Since XetDownloadStreamGroup::finish() is purely additive and existing callers won't adopt it right away, the Drop-based path here will be the common outcome for stream-group downloads — including ones that consumed every stream and fully succeeded. So outcome: dropped won't mean "abandoned"; for a while it'll mostly mean "succeeded but the caller didn't call finish()."
That's fine as long as everyone downstream knows to treat dropped as neutral rather than as a failure or an incomplete transfer — but it's an easy trap for a failure-rate or completion dashboard, and it's the metric the whole feature is meant to feed.
Since Drop already has the progress report in hand (self.report()), could it infer Outcome::Ok when the report shows everything completed, and reserve dropped for a genuinely partial transfer? That would keep dropped meaning "actually abandoned" and let success be read straight off the outcome. If the intent is instead that finish() becomes the norm and dropped stays a real signal, a note to that effect would help the query side.
| /// Backpressure. Documents submitted while this is at `max_in_flight` are dropped rather than | ||
| /// queued, so a hanging endpoint cannot accumulate tasks. | ||
| in_flight: Arc<AtomicUsize>, | ||
| max_in_flight: usize, |
There was a problem hiding this comment.
The in-flight cap lives on the sink, which is per-TransferTelemetry, i.e. per transfer. That bounds heartbeats within one long transfer nicely, but it doesn't bound the aggregate: a snapshot download fans out into many concurrent per-file transfers, each with its own sink and its own cap, so the process-wide number of in-flight telemetry POSTs is max_in_flight × (concurrent transfers) with no ceiling.
For best-effort telemetry that's not dangerous — worst case a burst of small POSTs that the endpoint sheds — but it's the opposite of where you'd want the backpressure: the heaviest telemetry moment (a wide snapshot) is exactly when there's no aggregate limit. Would a process-wide cap (a shared counter, or a shared sink) fit the model better than a per-transfer one? Not urgent, but it interacts with the per-file granularity in the other comments, so worth deciding deliberately rather than by default.
… note The note named the internal repository and PR number that adds the server-side `/v1/telemetry` endpoint. xet-core is public and that repository is not, so the reference is replaced with a neutral description of the companion change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…backend
The 429 and 500 descriptions in the OpenAPI spec said "Indexing saturated" and
"Indexing failed", which describes how the receiving service stores documents.
xet-core is public and that service is not, so this uses "Ingestion" instead -
matching both the endpoint's own summary ("Ingests a single client
transfer-performance document") and the wording already used in
`xet_client/src/cas_client/telemetry/sink.rs`.
The behaviour described is unchanged: 429 means the server is shedding load and
5xx means it is failing, and telemetry retries neither.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cargo Audit began failing on `event-listener 5.4.1`, which unconditionally implements Send/Sync for the `StackSlot` listener created by `listener!`, letting a `!Send` tag set via `Event::with_tag` cross a thread boundary. 5.4.2 is the patched release. The advisory is unrelated to this branch: main pins the same 5.4.1 and would fail the same check, it just has not re-run CI since the advisory landed in the database. Reaching us only through dev-dependencies (smol, async-std, httpmock), it never touched a shipped code path. Bumping beats an `.cargo/audit.toml` ignore entry here because a patched version exists, so there is no permanent exemption to carry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reaching the download session's `Drop` impl says only that nobody called
`finalize()`, not that the transfer failed. Since `XetDownloadStreamGroup::finish`
is additive and existing embedders have not adopted it, the overwhelmingly common
case there is a download that transferred everything and simply had no explicit
finalize - so hardcoding `Outcome::Dropped` made `dropped` mean "probably fine"
and left a failure-rate dashboard with nothing to measure. That is the one field
the feature exists to feed.
The Drop path now decides from progress: `ok` when the transfer actually
completed, `dropped` only when it did not, which restores `dropped` as a real
signal.
`GroupProgress::all_items_complete()` is the predicate, and it deliberately
checks more than the bytes. It requires every item's size to be *finalized* and
requires at least one item. Both guards are load-bearing:
- An open-ended stream range discovers its size incrementally, so `total_bytes`
tracks what the prefetcher has found while `bytes_completed` tracks what the
consumer has taken. A consumer that catches up to the prefetch frontier makes
the two equal mid-transfer, so a byte comparison alone would report an
abandoned stream as a success. `size_finalized` is set only once the
prefetcher reaches the real end of the file, which is the fact that
distinguishes "read to the end" from "stopped reading".
- An empty session compares 0 == 0, which would turn a session that never
started a download into a success.
Over-reporting `dropped` was recoverable; silently converting an abandoned
transfer into `ok` would not be, so both cases resolve to `dropped`.
Uploads keep reporting `aborted` on the same path: an abandoned upload never
committed, so nothing was durably transferred regardless of progress.
Reported by Rajat in review of #919.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Main moved while this PR was open (crates bumped to 1.6.0, plus the v2 shards simulation work from #884 and #910), leaving the PR conflicting. That also blocked CI outright: GitHub builds `pull_request` runs from the merge ref, so a PR it cannot merge gets no workflow runs at all. Both conflicts were in the simulation local server, where this branch's telemetry endpoint met main's new `/v2/shards` error-frame control. They are additive on both sides - a `telemetry_docs` field alongside a `shard_upload_error_frame` field - so every conflict resolves by keeping both. No behavior from either side is dropped. Verified on the merged tree: both CI clippy gates clean, `cargo fmt --check` clean, and the suites for xet-client (235), xet-data (389) and xet_pkg (295) all pass - including `test_local_server`, which exercises main's v2 shard error-frame path through the merged `ServerState`, and the telemetry tests that exercise this branch's endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The in-flight counter lived on the sink, and a sink belongs to one `TransferTelemetry` - one transfer. That bounded a single long transfer's heartbeats but not the aggregate: a snapshot download fans out into many concurrent per-file transfers, each with its own sink and its own counter, so the process-wide number of in-flight telemetry POSTs was `max_in_flight × concurrent transfers` with no ceiling. The backpressure was in the wrong place - the heaviest telemetry moment, a wide fan-out, was exactly the one with no limit. One `static IN_FLIGHT` counter shared by every sink makes `max_in_flight` a real ceiling. A shared *sink* would not work: it owns the endpoint URL and the authenticated HTTP client, so sharing one across auth contexts would be wrong. Sharing just the counter keeps each sink's identity intact. `max_in_flight`'s default moves 4 -> 32, which the mechanism change requires rather than merely suggests. As a per-transfer number 4 was reasonable; as a process-wide ceiling it would shed most of a wide snapshot's terminal documents and make this a coverage regression instead of a fix. 32 is sized for what actually bursts: one terminal document per transfer, with heartbeats only starting after `heartbeat_after`, so the realistic peak is a set of concurrent transfers finalizing together. The counter is reached through a `&'static AtomicUsize` field rather than the static directly, so tests can point a sink at an isolated counter and not contend with every other test in the binary. Reported by Rajat in review of #919. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design doc specifies the wire body's five keys as `time`, `event`, `session_id`, `user_agent`, `metrics`, and states that every key is snake_case - client-sent and server-stamped alike - with standardizing called out as something to follow through on both this PR and the endpoint's. The client was still emitting camelCase `userAgent`, the one spelling the doc wants retired. The server accepts both spellings, so this is safe either way; what it does not accept is a body carrying both, which is a duplicate-field 400. So exactly one must be sent, and it is now the snake_case one. Also replaces the "emits only the camelCase spelling" test with its inverse, and adds one asserting no envelope key contains a capital letter at all, so the convention is pinned rather than restated per key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`error_class` is a single closed vocabulary shared by both directions, but two of its values were unreachable from downloads. A CAS HTTP failure flattens into `XetError` before the download group classifies it, and every HTTP failure became `XetError::Network` - status discarded. So `telemetry_class()` could only ever say `network`, while the upload path classifies from `DataError`, inspects `reqwest::Error::status()`, and reports `rate_limited` and `server_error` properly. A 429 therefore meant two different things depending on direction, which defeats the point of a shared vocabulary and made the doc comment on `telemetry_class` - claiming both paths aggregate together - false. Adds `XetError::RateLimited` and `XetError::ServerError`, classified at the conversion boundary via the existing `ClientError::status()`, which also covers the middleware variant where the status is otherwise unreachable. Python-visible behavior is deliberately unchanged: both new variants map to `PyConnectionError`, exactly as `Network` did, so no caller's `except` clause changes. Only the message prefix differs. The enum is already `#[non_exhaustive]` and every external match has a wildcard, so this is additive. One gap is left on purpose and pinned by a test: a 404 arriving as a `reqwest` status still classifies as `network` rather than `not_found`. Routing it correctly would change the Python exception type callers catch, which is a user-visible change rather than a telemetry fix - worth doing deliberately, not as a side effect of this one. Reported by Cursor Bugbot on #919, and required by the design doc's closed `error_class` set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`test_envelope_has_exactly_the_five_contract_keys` already asserts the exact key set, `user_agent` included, so both removed tests were restating a fact it pins: an unexpected casing changes the key set and fails there first. Also drops the api_changes reference to the capital-letter test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`StatusCode::TOO_MANY_REQUESTS` names the status the check is about, and `is_server_error()` expresses the 5xx class through the type's own API rather than an open-coded `(500..600)` range. Same behavior, no numeric literals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every document carried a `dry_run` key that no consumer could learn anything from. `TransferTelemetry::maybe_new` returns `None` for a dry run, so no aggregator is built and nothing is ever emitted - as `test_dry_run_emits_nothing` asserts. Any document that exists therefore came from a non-dry-run transfer, and the field was structurally pinned to `false`. It was also absent from the design doc: the doc's payload-size table counts 40 keys on an upload document and 23 on a download, while the pinned sets here were 41 and 24. `dry_run` was the entire difference in both, so removing it lands exactly on the documented counts - good evidence the doc was measured against a key set that never had this field. Not free, either: the doc notes key names are over half a document's bytes, and this was a key plus a value in every single one. The stored field and the `dry_run()` accessor on `TransferTelemetry` go with it, since feeding this key was their only purpose. The `dry_run` parameter to `maybe_new` stays - it is what decides whether to build an aggregator at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jgodlew
left a comment
There was a problem hiding this comment.
Overall looks good! Just a couple of nits and a comment on the behavior of the heartbeats if we experience contention.
| let Some(metrics) = snapshot(seq) else { | ||
| return; |
There was a problem hiding this comment.
Should failing a snapshot cause all future heartbeats to be skipped?
From my understanding of the snapshot behavior, the snapshot can fail if this task is unable to try_lock() on the xorb_upload task. We could just skip this heartbeat and wait for the next one or sleep for sleep for a smaller time interval to retry?
There was a problem hiding this comment.
Good catch. I updated the task to skip the heartbeat instead of failing if snapshot fails.
`PyXetDownloadStreamGroup::__exit__` called `finish` unconditionally, ignoring `exc_type`. `finish` finalizes with `Outcome::Ok`, so a `with` block that raised recorded the failed transfer as a successful one - and because `finish` sets `terminal_sent`, the `Drop` path never got to correct it. Failure-rate telemetry for stream-group downloads was wrong in exactly the case worth measuring. `__exit__` now branches on `exc_type`, which is what the upload-commit and file-download-group context managers already did: normal exit finishes, an exception aborts, and abort's own error is swallowed with a warning so it can never mask the exception being propagated. The gap was that `XetDownloadStreamGroup` had no `abort` - only `finish` - so `__exit__` had nothing else to call. Added on both sides, built on the existing `FileDownloadSession::abort_active_streams()`. `abort` deliberately emits no telemetry, matching `XetFileDownloadGroup::abort`. The session is left unfinalized so its `Drop` derives the outcome from what actually transferred: `dropped` for a genuinely partial transfer, `ok` only when every stream really was consumed to its end. That last case is the honest answer when the transfer completed and the exception came from the caller's own code - and it is only available because the `Drop` path now infers from progress rather than hardcoding `dropped`. Reporting `error` here would have been wrong in the other direction: the exception is frequently a bug in the caller's loop body or a `KeyboardInterrupt`, and classifying those as transfer failures would inflate the same metric. The doc reserves `cancelled` for a real user interrupt. Accepted tradeoff, unchanged from the sibling paths: `Drop`'s send is detached, so abandoned transfers are less reliably delivered than finished ones. Already true of every abandon path and documented as such. Reported by Cursor Bugbot on #919. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…the sim server `post_telemetry` unwrapped the `telemetry_docs` mutex with `.expect`, so a poisoned lock would panic inside a request handler. Simulation code, but a panic there surfaces as a hung or mysteriously failing test rather than as the condition it actually is; every other failure in this handler is reported as a status code. Returns `INTERNAL_SERVER_ERROR` instead, which the client treats as any other server-side telemetry failure: swallowed and not retried. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… ceiling Both permit-acquisition paths fed `record_concurrency` the controller's `total_permits()` - its configured limit - so `peak_concurrency` reported what the adaptive controller was *willing* to run rather than what was actually in flight. On a transfer that never saturates its allowance the two are unrelated, which defeats the metric's purpose: diagnosing a throughput regression means knowing the parallelism that was really achieved. `active_permits()` is read immediately after the permit is acquired, so it counts in-flight connections including the one just taken. The field's own doc already claimed "highest concurrency observed"; now it is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every line in the sink was `debug!`, so at default verbosity a client log gave no sign of whether telemetry was on, configured correctly, or reaching the endpoint at all - the first question asked when a transfer is missing from the data. The accepted-document line is now `info!`. The failure paths stay at `debug!`: telemetry is best-effort and never retried, so a dropped document is not something a user should be told about during their transfer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A send that never reaches the endpoint at all - DNS, TLS, connection refused, timeout - is the other half of the question "is telemetry working?", and at `debug!` it was as invisible as the success it replaces. Both ends of the POST are now visible at default verbosity. The rejected-by-server path stays at `debug!`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… Sync `telemetry_of` and `telemetry_of_download` had identical bodies and existed only because the upload session held `Arc<dyn Client + Send + Sync>` while the download session held `Arc<dyn Client>`. `Client` declares `Send + Sync` as supertraits, so those spell the same type and the split bought nothing. The upload session's field and `client()` accessor drop the redundant bound, and the download-specific wrapper is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six telemetry call sites needed only `n_files`, and each got it by calling `item_reports()` - which locks the items map, builds an `ItemProgressReport` per item (four atomic loads apiece) into a fresh `HashMap`, and then throws all of it away for a `len()`. On the heartbeat paths that ran once per beat per transfer. `GroupProgress::n_items()` takes the same lock and reads the map's length, forwarded through `UploadGroupProgress` and both sessions. Returns `usize` like `len()`, cast to `u64` at the payload boundary, which is what the shard counts already do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI's fmt job runs nightly rustfmt over both manifests, and three files had drifted from it: - `core/mod.rs`: the `register_pre_shutdown_drain` re-export I added in cc9a9c4 was not in `group_imports` order. - `download_stream_group.rs` and `py_download_stream_group.rs`: `finish`'s docstrings were wrapped at ~95 columns rather than the configured `comment_width = 120`. All three would have failed `cargo fmt --all -- --check`. No content changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`XetConfig::with_env_overrides` read `HF_HUB_DISABLE_TELEMETRY` and `HF_HUB_OFFLINE` and force-disabled telemetry when either parsed as truthy. That put `huggingface_hub`'s environment contract inside `xet_runtime`, which has no business knowing those variables exist: the library that owns them can read them itself and pass `telemetry.enabled = false` through the `XetConfig` it already constructs for `XetSession`. Removes `telemetry_opted_out`, `TELEMETRY_OPT_OUT_VARS`, the override block, and the six tests that covered the env-var polarity. `HF_XET_TELEMETRY_ENABLED` remains the only environment control. Note this drops the opt-out entirely until the `huggingface_hub` side lands, so between the two changes those two variables no longer suppress reporting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`XetDownloadStreamGroup::abort` cancelled the active streams but left the group's task subtree Running, so a caller could open new streams on an abandoned group - while `download_stream` and `download_unordered_stream` both documented `XetError::UserCancelled` for exactly that case, and `XetFileDownloadGroup::abort` had always closed its group. `abort` now calls `cancel_subtree()` first, so `download_stream`, `download_unordered_stream`, and `finish` all return `UserCancelled` afterwards. Only the subtree under this group is cancelled - `XetSession::abort` already does the same one level up - so aborting one group leaves the rest of the session running. Cancellation is not finalization, so the telemetry contract is unchanged: the session stays unfinalized and its `Drop` still derives the outcome from what transferred. `stream_group_abort_does_not_report_success` continues to see `dropped` for an aborted partial transfer. Python-side coverage follows the same shape. `test_context_manager_aborts_on_exception` asserted the group was still open after a raising `with`; both `__exit__` branches now close it, so it asserts the branch instead - `cancelled` for abort against `already finalized` for finish. Adds `test_abort_closes_the_group`, `test_abort_twice_is_a_no_op`, and `test_abort_makes_finish_fail` - the analog of `TestFileDownloadGroup`'s test of the same name, which only now has the behavior to assert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fea0f48. Configure here.
…ession's `XetUploadCommit::commit` finalizes each file's ingestion, keeps the first error, finalizes the session, and then returns that error. Telemetry was emitted inside the session's finalize and classified only from its result, so a commit whose file ingestion failed but whose session finalized cleanly reported `outcome: ok` while returning an error to its caller. Failure-rate telemetry missed exactly those commits. `commit` now passes the failure it is already holding to `finalize_with_report_as`, which reports it in place of `ok`. A failure in the session's own finalize still wins, matching which of the two errors `commit` returns. Emission stays inside `finalize_impl` rather than moving up to `commit`: the session is what every upload path goes through, and hooking at the caller layer is how the download side once lost its reporting entirely. `FileUploadSession::finalize_with_report_as` mirrors `FileDownloadSession::finalize_with`, which exists for the same reason - the caller that knows how the transfer ended holds a `XetError`, which `xet_data` cannot classify. Note the end-to-end path has no test. Reaching it needs a per-file `finalize_ingestion` failure alongside a healthy session finalize, and there is no public way to induce one: `abort_task` is `pub(super)`, and a provided SHA-256 is trusted rather than verified. The classification itself is covered by `test_reported_failure_overrides_a_clean_finalize` and `test_finalize_failure_wins_over_a_reported_failure`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the download `FileUploadSession::finalize_with_report_as` and `FileDownloadSession::finalize_with` do the same job - finalize while reporting an outcome the session cannot derive on its own - so they should read the same at a call site. Renamed to `finalize_with`; the upload's extra return value is a signature detail, not a different operation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`finalize_inner` took the metrics out of the session and only then made two fallible calls - `session_file_info_list` and `upload_and_register_session_shards`. When either failed, the error path in `finalize_impl` read the session's mutex back and found `DeduplicationMetrics::default()`, so a shard-upload failure reported zeroed dedup, chunk, and xorb-byte counts: the failures most worth measuring carried the least data. The take now happens after both calls. The ordering constraint it was written for still holds - it has to follow the xorb-upload join, because those tasks record transmitted bytes into the session only once their CAS request resolves. Nothing writes to the metrics after that join (the remaining writers are the file-cleaner merges during ingestion, and the shard interface cannot reach them), so taking later captures the same values. Failures before the take - `process_aggregated_data_as_xorb` and the join itself - already reported correctly; this extends that to the two after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| heartbeats only begin after `heartbeat_after`, so the realistic burst is a set of concurrent | ||
| transfers finalizing together. | ||
|
|
||
| `HF_HUB_DISABLE_TELEMETRY` and `HF_HUB_OFFLINE` also force it off, and win over |
There was a problem hiding this comment.
Let's remove mentioning of HF_HUB env vars as disabling telemetry is done programmatically.
| To turn it off, use any of: | ||
|
|
||
| ```bash | ||
| HF_XET_TELEMETRY_ENABLED=0 # hf-xet specific |
There was a problem hiding this comment.
Let's remove mentioning of HF_HUB env vars as disabling telemetry is done programmatically.
seanses
left a comment
There was a problem hiding this comment.
LGTM! Let's fix the above two doc issues and merge!
Part 1 of 6 of the client transfer telemetry stack, split out of #919 for review. Each PR in the stack compiles and passes CI on its own. Adds the telemetry config group (enabled, heartbeat, timeouts, in-flight cap) and registers it with all_config_groups!, so it reaches callers as ctx.config.telemetry. Adds register_pre_shutdown_drain, a process-wide hook that XetRuntime::Drop calls while the runtime can still drive tasks. Shutting a runtime down cancels pending tasks rather than completing them, so fire-and-forget work is lost unless something waits for it first; this is the only point that knows both that the runtime is alive and that it is about to die. It fires only on the synchronous path for an owned thread pool - blocking inside an async context is what the neighbouring branch exists to avoid, and an external runtime outlives us, so its tasks are never cancelled here. EnvVarGuard::unset supports the config tests, which must assert default behaviour without being perturbed by a value the developer happens to export. No consumer yet; xet_client's telemetry sink is the first, in the next PR. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Infrastructure-only: new config defaults and a narrow shutdown hook with no telemetry consumer yet; owned-runtime drop may block briefly when a drain is registered later. > > **Overview** > Introduces **`telemetry`** as a new `XetConfig` group (wired through `all_config_groups!` and `TelemetryConfig`) so upcoming transfer telemetry can read **`ctx.config.telemetry`**. The group defines enablement (default on), heartbeat timing, POST timeouts, a bounded **`finalize()`** flush wait, and a **process-wide** in-flight cap with `HF_XET_*` env overrides, plus serial tests that use **`EnvVarGuard::unset`**. > > Adds **`register_pre_shutdown_drain`**: a one-shot, process-wide hook invoked from **`XetRuntime::Drop`** on the synchronous owned-runtime path **before** Tokio shutdown, so best-effort detached work (e.g. telemetry POSTs) can finish instead of being cancelled. Skipped for external runtimes, forked children, and async-context drops. > > **`EnvVarGuard::unset`** is new for tests that need clean default env state. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 50b63b8. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…tor (#933) Part 2 of 6 of the client transfer telemetry stack, split out of #919 for review. Each PR in the stack compiles and passes CI on its own. Adds cas_client::telemetry: the wire envelope, a batching HTTP sink with a process-wide in-flight cap, and TransferTelemetry, which accumulates one transfer's counters and owns its identity and timing. The sink registers xet_runtime's pre-shutdown drain so in-flight documents are delivered rather than cancelled when an owned runtime is dropped. The in-flight cap is process-wide rather than per-client, so a caller holding several clients cannot multiply the ceiling. RemoteClient constructs telemetry from the already-authenticated http client rather than building its own: a second build_auth_http_client would create a second AuthMiddleware with its own TokenProvider, giving telemetry an independent token-refresh cycle against the Hub. It records concurrency from active permits at acquisition time, which measures what was actually in flight rather than the configured ceiling. Client::transfer_telemetry is defaulted to None so only RemoteClient implements it; the local, in-memory, and simulation clients inherit the default and are silently excluded from reporting. The whole module is gated off wasm - there is no XetRuntime::spawn there, so there is no way to report without blocking a transfer - which also keeps chrono's clock off a target where it would need wasmbind. **Review note:** this is the biggest PR in the stack (+1081). Splitting the transport from the aggregator was considered and rejected: it required a throwaway stub `telemetry/mod.rs` in the lower PR that the upper one replaced wholesale, which every later rebase would conflict on. Read top-down from `telemetry/mod.rs`; `sink.rs` and `envelope.rs` are its two collaborators. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds background HTTP and shutdown draining on the transfer hot path, but failures are swallowed, telemetry is opt-in via config, and wasm builds omit the module entirely. > > **Overview** > Introduces **`cas_client::telemetry`** (native targets only): a **`TelemetryEnvelope`** for `POST /v1/telemetry`, **`TransferTelemetry`** for per-transfer identity/timing/peak concurrency, and **`TelemetrySink`** for fire-and-forget heartbeats plus bounded awaited terminal flushes. > > **`RemoteClient`** optionally constructs telemetry via `TransferTelemetry::maybe_new` (config on, not dry-run, http/https endpoint), reuses the existing authenticated HTTP client, records active upload/download permits at acquisition, and exposes the aggregator through **`Client::transfer_telemetry`** (default `None` for other clients). **`chrono`** / **`uuid`** are non-wasm-only dependencies. > > The sink enforces a **process-wide in-flight cap** (drops when saturated, no retries), registers **`register_pre_shutdown_drain`** so detached posts can finish before runtime shutdown, and supports periodic heartbeats that stop once a single terminal summary is emitted. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 091ef89. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Part 3 of 6 of the client transfer telemetry stack, split out of #919 for review. Each PR in the stack compiles and passes CI on its own. Defines what a transfer report contains: the outcome and error-class vocabulary, the shared identity and common metrics, and the upload- and download-specific metric sets, with the OpenAPI description of the endpoint that receives them. These definitions live in xet_data rather than xet_client because this is the only layer that can see DeduplicationMetrics and GroupProgressReport. The outcome vocabulary stays ungated on wasm even though the payloads do not: it appears in FileDownloadSession's public signatures, and gating it would push a cfg onto every caller that merely names an outcome. Nothing emits these yet - the emit path and its session hooks are the next PR. **Review note:** `xet_data/src/telemetry/mod.rs` declares only `outcome` and `payload` here. Part 4 adds `mod emit;` and its re-exports — that file is additive across the two PRs, not replaced. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Additive API spec and library types with no runtime behavior change until a later PR wires emission; schema is guarded by tests but future key/type changes would affect analytics consumers. > > **Overview** > Introduces the **client transfer telemetry contract** ahead of wiring: OpenAPI documents fire-and-forget **`POST /v1/telemetry`** (read scope) with a **`TelemetryEnvelope`** whose `metrics` map is intentionally open-ended and points to Rust as the source of truth. > > Adds **`xet_data::telemetry`** with a **wasm-universal outcome layer** (`Outcome`, stable wire strings, `error_class` / `classify_error` over `DataError`) and **non-wasm payload builders** that flatten **`CommonMetrics`**, **`UploadMetrics`** (dedup/shard/ingest timing), and **`DownloadMetrics`** from `GroupProgressReport`, `DeduplicationMetrics`, and `TransferTelemetry` snapshots. Payload construction enforces **fixed key sets**, **scalar-only JSON**, and **finite ratios/rates**; tests lock upload/download keys, types, and no-PII rules. > > **No emission in this PR**—only types, serializers, and API spec; the next stack piece hooks sessions to send these documents. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a1e7ab7. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Part 4 of 6 of the client transfer telemetry stack, split out of #919 for review. Each PR in the stack compiles and passes CI on its own. Wires the payloads to real transfers. Upload and download sessions report a terminal document on finalize, an abandoned one from Drop, and periodic heartbeats for transfers long enough to be worth observing in flight. A download's Drop outcome is inferred from its progress rather than assumed: GroupProgress::all_items_complete checks size_finalized as well as the byte counts, because for an open-ended stream range total_bytes tracks only what the prefetcher has found so far. A consumer that catches up to the prefetch frontier makes the counts equal mid-transfer, so the byte comparison alone would report an abandoned download as a finished one. n_items counts registered items without snapshotting every one of them, which matters on the heartbeat path where the count is all that is wanted. Adds the simulation server's telemetry receive route, which exists so these integration tests can assert what is actually delivered. The tests also guard the Client::transfer_telemetry override: it has a default body, so a mistyped signature would compile and silently never be called. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches session lifecycle (`finalize`, `Drop`, heartbeats) and outcome classification for metrics; behavior is best-effort and should not fail transfers, but mis-inferred Drop outcomes or duplicate/missing events would skew observability. > > **Overview** > **Connects real transfers to client transfer telemetry** (non-wasm): upload and download sessions now start periodic heartbeats at creation, send a terminal summary on `finalize` / `finalize_with`, and send a best-effort detached terminal when dropped without finalizing. > > Upload telemetry runs inside `FileUploadSession::finalize_impl` (including optional **reported failure** when the caller already knows the commit failed), tracks `ingest_ms` / `finalize_ms`, and uses non-blocking dedup snapshots for heartbeats and `Drop`. Download adds `finalize_with` for explicit outcomes; `Drop` infers **ok vs dropped** via new `GroupProgress::all_items_complete()` (requires `size_finalized`, not byte equality alone). > > Adds **`xet_data::telemetry::emit`** to build payloads and call `TransferTelemetry`, plus **`n_items`** on progress types for cheap counts. > > The **simulation local server** gains `POST /v1/telemetry` with in-memory recorded docs exposed on `LocalTestServer` / `TestEnvironment` for **`test_transfer_telemetry`** end-to-end wire-shape and gating tests (disabled telemetry, dry run, no double-emit after finalize). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 0d51cbe. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…ps (#936) Part 5 of 6 of the client transfer telemetry stack, split out of #919 for review. Each PR in the stack compiles and passes CI on its own. Splits RateLimited (429) and ServerError (5xx) out of the catch-all Network variant. A 429 is not a network fault - it is the server asking for less traffic - and collapsing them made rate_limited and server_error unreachable for downloads while uploads still reported them, so the same status meant two different things depending on direction. Both map to the same Python exception as Network, so this is not a user-visible change. XetError gains a telemetry classifier mapping the surviving categories onto the same coarse vocabulary xet_data uses, so documents from this path aggregate with those classified further down. One gap is deliberate: a 404 arriving as a reqwest status still classifies as network here, because routing it to NotFound would change the exception type callers see. The session groups report their own outcome: an upload commit reports its own failure rather than only the session's, a group closes on abort as its callers already promise, and a with-block that raises aborts rather than finishes. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes transfer telemetry emission and error classification across download/upload group APIs and Python bindings paths; behavior is well-tested but affects observability and lifecycle semantics (finish vs abort vs drop). > > **Overview** > **Telemetry and error taxonomy** — `XetError` adds `RateLimited` and `ServerError` for HTTP 429/5xx from `ClientError`, plus `telemetry_class()` so flattened public errors use the same coarse classes as `xet_data` (Python still maps those variants to `PyConnectionError`). > > **Session finalization** — File download groups, legacy `download_async`, and upload commits now call `finalize` / `finalize_with` with the real outcome before returning errors, so failed transfers are not silently omitted from metrics. Upload commits avoid reporting `ok` when a per-file error will be returned. > > **Download stream groups** — New optional `finish` / `finish_blocking` (explicit success telemetry and group close) and `abort` (cancel subtree, no success telemetry; `Drop` still infers partial vs complete). Integration tests assert `xet_download_summary` through the public group APIs, not direct session `finalize`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 010ff32. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…#937) Part 6 of 6 of the client transfer telemetry stack, split out of #919 for review. Each PR in the stack compiles and passes CI on its own. Makes the Python download stream group abort rather than finish when its with block raises, so an exception is reported as an abandoned transfer instead of a successful one, and documents the telemetry the client now emits along with how to turn it off. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Mostly additive Python API surface and documentation. Telemetry outcome reporting changes only for stream-group context-manager exception paths; transfer behavior is otherwise unchanged. > > **Overview** > Exposes **`finish()`** and **`abort()`** on the Python `XetDownloadStreamGroup`, plus context-manager support, so callers can explicitly close a stream group and report transfer telemetry. > > On a clean `with` exit the group finishes; on an exception it **aborts** instead, leaving the session unfinalized so `Drop` derives the outcome from what actually transferred rather than recording a failed download as `ok`. > > Also documents client transfer telemetry in the README (what is sent, how to disable it, and tuning knobs) and adds the stack’s API-change note. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 9bf2fc6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Adds client-side transfer performance telemetry so we can detect and alert on throughput regressions shipped in an
hf-xetrelease. Each upload or download reports one summary document toPOST /v1/telemetry, the endpoint added by a companion server-side change.This is PR 1 of 2. A follow-up (#922) adds the generated schema (
telemetry/metrics.schema.json) and a CI compatibility gate. See Rollout — there is a decision needed before this is enabled against a real index.Verified end-to-end, not just in tests: a real
hfCLI upload and download against a local Hub + a local CAS server carrying the new endpoint. Details in Verification — that exercise is what surfaced the download gap fixed below.Why
Server-side metrics cover CAS request latency but not end-to-end client throughput, dedup effectiveness, or where a transfer's wall time goes. Without that we can't tell a slow client from a slow server, and a regression in a released client is invisible.
Design
One hook point covers every surface. All four client entry points bottom out in
FileUploadSession/FileDownloadSession, so the measurement lives entirely inxet_dataandxet_client:XetUploadCommit::commit,XetFileDownloadGroup::finishdata_client(what shippedhuggingface_hubcalls today)git_xet/xtoolXetDownloadStreamGroupfinish(), plusDropas the fallbackNotable choices:
Client::transfer_telemetry()has a defaultNonebody, so 6 of the 7 implementors are untouched and local/memory/simulation clients are excluded for free.RemoteClient's authenticated HTTP client rather than building one. A secondbuild_auth_http_clientwould create a secondAuthMiddlewarewith its ownTokenProvider, giving telemetry an independent token-refresh cycle against the Hub.TransferTelemetryis direction-agnostic.RemoteClienthas no notion of direction, and threading one through would have touched all 18 of its construction sites to benefit 2.xet_dataknows which kind of session it holds and supplies it when building the payload.XetRuntimehas nospawnthere, so there is no way to report without blocking a transfer. The outcome vocabulary (telemetry::outcome) does compile everywhere, because it appears inFileDownloadSession's public signatures and gating it would push#[cfg]onto every caller that merely names an outcome.Making the production paths actually emit
The measurement side above was not sufficient on its own. Wiring it up needed changes in
xet_pkgandhf_xet:XetFileDownloadGroup::finish/finish_blockingand legacydownload_asyncnow finalize the download session, on the error path as well as the success path. Nothing previously calledFileDownloadSession::finalize(), which is the only route to a download document — so downloads reported nothing at all through the Python bindings while every test passed.huggingface_hubwas already doing the right thing on its side; the Rustfinish_blockingsimply read the progress report and returned.FileDownloadSession::finalize_with(outcome, error_class)—finalize()hardcodedOk(())as the result, so a download could only ever reportoutcome: ok. The plumbing for error outcomes existed but was unreachable, which would have left download failure-rate alerting reading zero forever.XetError::telemetry_class()mapsxet_pkg's error categories onto the same coarse class vocabularyxet_datauses, so both paths aggregate together.XetDownloadStreamGroup::finish(plusfinish()and context-manager support on the Python class). Streams are consumed independently, so the group cannot detect completion itself. Purely additive — a group that is never finished behaves exactly as before and still reports, just asdropped; no existing caller has to change.Dropfallback no longer requires an ambient tokio runtime. The send is spawned on theXetRuntime's own stored handle, soHandle::try_current()was never needed — and requiring it silently disabled the fallback for embedders that release the lastArcfrom a foreign thread, which is exactly what the Python bindings do.Fire-and-forget
Never retried (a 429 is the server shedding load), 5s per-request timeout, an in-flight cap that drops rather than queues, and every failure swallowed at DEBUG.
One deliberate exception: the terminal document is awaited, bounded by
final_flush_timeout(default 2s). A fully detached final send is usually lost, because host processes routinely exit within milliseconds of a transfer returning. It runs after all transfer work is done — teardown, not the data path — andHF_XET_TELEMETRY_FINAL_FLUSH_TIMEOUT=0makes it fully detached. There's a wiremock test asserting a hanging endpoint can't hold a transfer past the budget.The removed
hf_xet/src/telemetry.rs(#441) used a tracing layer, a process-globalOnceLock, its own runtime, and aMutexheld across sends — which hung aroundos.fork(). This design has none of those; all state hangs offXetContext/RemoteClientand dies with them.Privacy
The payload carries no file names, paths, hashes, repository ids, or user ids. Enabled by default, and disabled by
HF_XET_TELEMETRY_ENABLED=0,HF_HUB_DISABLE_TELEMETRY, orHF_HUB_OFFLINE— the opt-outs win over an explicit enable.Worth stating plainly: the resulting documents are user-identifiable server-side, because cas_server attaches the JWT claim set (
userId,repoId) andclientIp. That's a property of the ingest endpoint, not of this change, but it shouldn't be implied away by the client-side "no PII" claim.Metrics
~26 common keys (byte counts, throughput, duration, outcome, error class, concurrency, client version/os/arch, endpoint host only), plus 16 upload-only from
DeduplicationMetricsand shard progress, andexpansion_ratiofor downloads.xet_data/src/telemetry/payload.rsis the source of truth.The key set is a contract. Consumers assign each property a type on first sight and cannot change it in place, so adding a key is safe but changing an existing key's JSON type breaks ingestion for every document carrying it, and recovering means rebuilding the stored data. Two tests enforce this. Relatedly, every
f64goes through a finite guard —serde_jsonrenders NaN and infinity asnull, and one such document poisons that property's type for a consumer.Testing
~70 tests: 9 config gating, 24 in
xet_client(envelope, sink, in-flight cap, heartbeat weak-reference, four against wiremock), 18 payload/outcome/emit, and 19 end-to-end against the simulation server (13 inxet_data, 6 inxet_pkg).Two failure modes these are shaped around, both of which actually occurred:
transfer_telemetryhas a default body, so an override with a mistyped signature would compile cleanly and silently never be called.xet_data/tests/test_transfer_telemetry.rsdrives a real transfer through a real HTTP server to catch that.FileDownloadSessiondirectly cannot see this, because they callfinalize()themselves.xet_pkg/tests/test_download_telemetry.rsgoes throughXetFileDownloadGroup::finish_blocking()and asserts on what the server received. Please keep new coverage at that altitude.Both new regression tests were confirmed to fail when their fix is reverted, not merely to pass with it.
Run as CI runs it:
cargo test --features "strict simulation internal-tools"— all greencargo clippy -r -- -D warnings, for the workspace and forhf_xet— cleancargo +nightly fmt— cleanhf_xet_wasm+hf_xet_thin_wasm(wasm32) buildVerification
Beyond the test suite, the full path was exercised against real infrastructure:
hfCLI →huggingface_hub1.26 → this branch'shf-xet→ a local Hub → a local CAS server with the new endpoint → the receiving datastore.Confirmed there:
env,casVersion, the JWT claim set, andclientIpfrom the rightmostX-Forwarded-Forhop.telemetry/metrics.schema.json.n_files> 1); download emits one per file, becausehf_hub_downloadopens a fresh download group per file — son_filesis always 1 there and a 200-shard model produces 200 documents.max_in_flightis per-TransferTelemetry, i.e. per transfer, not a global cap. It bounds heartbeats within one long transfer but does not throttle a concurrent multi-file snapshot download.Rollout
Point the ingestion side at a scratch destination for now. Property types are pinned by the first documents that carry them and cannot be changed in place, and this PR starts writing before any storage mapping is designed — so whatever those early documents imply is what we are stuck with.
What automatic type inference actually produced when measured, rather than assumed:
throughput_bpsserializing as0.0pinning the property to an integer type) does not fire, becauseserde_jsonalways renders anf64with a decimal point. It would fire if a metric were ever changed fromf64to an integer type: a bare0pins the property, and every later float is then silently truncated with no error surfaced.transfer_idandsessionIdand low-cardinality enums likeoutcome. Those want exact-match only.71910674.2857stores as71910672.0). Relative error is ~1e-7, so aggregations are unaffected, but exact values are not preserved. Double precision is a choice worth making deliberately.None of that is fixable in place, hence the scratch destination. Deriving the storage mapping is the ingestion side's call — this repo publishes JSON types via
metrics.schema.jsonand deliberately does not carry the receiving service's storage layout.This PR alone cannot deliver the regression alerting that motivates the work — until the ingestion side types these fields as numbers, they are not aggregatable. That work and this shouldn't be left to drift apart.
Follow-ups
huggingface_hubchange (one group for many files), not a xet-core one.RetryWrapperis the chokepoint; scoped in the plan.🤖 Generated with Claude Code
Note
Medium Risk
Touches core upload/download finalize and Python stream-group lifecycle with default-on reporting, but sends are bounded, non-blocking for data movement, and carry no client-side PII; mis-finalization or dropped telemetry would affect observability more than transfer correctness.
Overview
Adds best-effort transfer performance telemetry that POSTs one terminal summary (and optional heartbeats) per upload/download to
POST /v1/telemetry, with a newtelemetryconfig group,HF_HUB_*opt-outs, and README/OpenAPI/docs.xet-clientgainsTransferTelemetry(non-wasm): snake_case envelope, fire-and-forget sink with process-wide in-flight cap, bounded terminal flush, and a defaultClient::transfer_telemetry()hook implemented onRemoteClient(shared auth HTTP client, concurrency sampling).xet-databuilds flat metric payloads from progress/dedup state, starts heartbeats from sessions, emits on finalize (includingfinalize_with/ uploadreported_failure), andDropwhen sessions are abandoned;GroupProgress::all_items_complete()drives download outcomes whenfinish()was never called.Production wiring fixes: download groups and legacy download paths now finalize the download session (so Python-bound downloads actually report);
XetDownloadStreamGroupexposesfinish/abortand Python__exit__no longer reportsokon exceptions.Tests/simulation: local server records telemetry docs; integration tests assert end-to-end delivery through
finish_blocking.Reviewed by Cursor Bugbot for commit 2c45835. Bugbot is set up for automated code reviews on this repo. Configure here.