-
Notifications
You must be signed in to change notification settings - Fork 102
feat(telemetry): client transfer performance telemetry #919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
6f46dcb
feat(telemetry): config, envelope, sink, and payload definitions
sirahd 984be35
feat(telemetry): emit from session finalize, Drop, and heartbeats
sirahd e275ac0
feat(telemetry): simulation route, integration tests, and docs
sirahd a701369
docs(telemetry): keep the receiving service's storage layout out of t…
sirahd 28f2f6d
fix(telemetry): downloads never reported; no caller finalized the ses…
sirahd a95b389
fix(telemetry): restore the wasm and Windows builds
sirahd 0983538
docs(telemetry): drop the private repo reference from the api_changes…
sirahd ec6715b
docs(telemetry): describe the endpoint's failures without naming its …
sirahd 06ac796
fix(deps): bump event-listener to 5.4.2 for RUSTSEC-2026-0221
sirahd 9e02529
fix(telemetry): infer a download's Drop outcome from its progress
sirahd ac3adae
Merge main into sira/client-transfer-telemetry
sirahd d639ce6
fix(telemetry): make the in-flight cap process-wide
sirahd 4cd858c
fix(telemetry): emit the envelope's user_agent in snake_case
sirahd aeec419
fix(telemetry): preserve HTTP status so downloads can report 429 and 5xx
sirahd 72681cb
test(telemetry): drop the two redundant envelope casing tests
sirahd d1a9f2c
refactor(telemetry): match on StatusCode instead of a u16 conversion
sirahd 6c824da
fix(telemetry): drop the dry_run metric, which could only ever be false
sirahd 173d45a
fix(telemetry): abort rather than finish when a with block raises
sirahd 218528b
docs(telemetry): correct what skipping finish() actually reports
sirahd a05ec97
docs(openapi): use snake_case for the telemetry fields in prose and e…
sirahd cc9a9c4
fix(telemetry): drain in-flight documents before the runtime shuts down
sirahd 8af0427
fix(telemetry): a contended metrics snapshot skips one beat, not ever…
sirahd 3d48e2d
chore: refresh the sub-workspace lockfiles
sirahd 9cf0fbf
fix(telemetry): keep stream-group finish and abort responsive to signals
sirahd 94e478c
test(telemetry): assert what finish and abort leave behind on a strea…
sirahd 388238b
fix(telemetry): return a 500 rather than panic on a poisoned lock in …
sirahd b7ddd15
fix(telemetry): measure peak_concurrency from active permits, not the…
sirahd 92fbc1f
fix(telemetry): log an accepted document at info!
sirahd e33f4fe
fix(telemetry): log a failed send at info! as well
sirahd cae4cef
refactor(telemetry): one telemetry_of, since Client is already Send +…
sirahd 3f1119a
perf(telemetry): count items without snapshotting every one of them
sirahd 6098134
style: apply nightly rustfmt
sirahd f039f8b
refactor(telemetry): drop the HF_HUB env-var opt-out
sirahd fea0f48
fix(telemetry): close the group on abort, as its callers already promise
sirahd edad220
fix(telemetry): report an upload commit's own failure, not just the s…
sirahd cdbd48d
refactor(telemetry): name the upload override finalize_with, matching…
sirahd 2c45835
fix(telemetry): keep dedup metrics readable when finalize fails late
sirahd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| # Client transfer telemetry | ||
|
|
||
| **Date**: 2026-07-28 | ||
| **Crates**: `xet-runtime` (new `telemetry` config group), `xet-client` | ||
| (`cas_client::telemetry`, `Client` trait), `xet-data` (`telemetry` module, session hooks) | ||
|
|
||
| ## What changed | ||
|
|
||
| The client now reports one performance summary per transfer to `POST /v1/telemetry` on the CAS | ||
| server, where a companion change adds the endpoint. Reporting is best-effort: it is never retried, | ||
| never surfaces an error, and never blocks data movement. | ||
|
|
||
| ### Wire format: the envelope is entirely snake_case | ||
|
|
||
| The body carries exactly five keys — `time`, `event`, `session_id`, `user_agent`, `metrics` — and | ||
| **every one is snake_case**, matching the naming convention across the whole document (envelope, | ||
| `metrics`, and the fields the server stamps itself). | ||
|
|
||
| `user_agent` was previously emitted as camelCase `userAgent`. The server accepts that spelling as a | ||
| serde alias for older clients, so both work, but a body carrying **both** is rejected as a | ||
| duplicate-field 400 — so exactly one must be sent, and it is now the snake_case one. The key set is | ||
| pinned by `test_envelope_has_exactly_the_five_contract_keys`. | ||
|
|
||
| `metrics` carries **40 keys on an upload document and 23 on a download**, pinned by | ||
| `UPLOAD_KEYS`/`DOWNLOAD_KEYS`. There is no `dry_run` key: a dry run never builds a | ||
| `TransferTelemetry` at all (`maybe_new` returns `None`), so the field could only ever serialize | ||
| `false` — a constant in every document, and key names are over half a document's bytes. | ||
|
|
||
| ### New config group: `telemetry` | ||
|
|
||
| | Field | Env var | Default | | ||
| |---|---|---| | ||
| | `enabled` | `HF_XET_TELEMETRY_ENABLED` | `true` | | ||
| | `heartbeat_after` | `HF_XET_TELEMETRY_HEARTBEAT_AFTER` | `300s` | | ||
| | `heartbeat_interval` | `HF_XET_TELEMETRY_HEARTBEAT_INTERVAL` | `300s` | | ||
| | `request_timeout` | `HF_XET_TELEMETRY_REQUEST_TIMEOUT` | `5s` | | ||
| | `final_flush_timeout` | `HF_XET_TELEMETRY_FINAL_FLUSH_TIMEOUT` | `2s` | | ||
| | `max_in_flight` | `HF_XET_TELEMETRY_MAX_IN_FLIGHT` | `32` | | ||
|
|
||
| `max_in_flight` is a **process-wide** ceiling: one counter is shared by every sink, so the total | ||
| number of in-flight telemetry POSTs is bounded no matter how many transfers run at once. A per-sink | ||
| counter would have bounded each transfer separately and multiplied by the transfer count, which puts | ||
| the backpressure in the wrong place — a snapshot download fans out into many concurrent per-file | ||
| transfers, so the heaviest telemetry moment would have been the one with no aggregate limit. The | ||
| default is sized as a process-wide number accordingly: a transfer emits one terminal document and | ||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove mentioning of |
||
| `HF_XET_TELEMETRY_ENABLED=1`. These are applied at the end of `XetConfig::with_env_overrides` | ||
| rather than through `ENVIRONMENT_NAME_ALIASES`, because that table maps names with identical | ||
| polarity and these are inverted. | ||
|
|
||
| ### `Client` trait gained a method — **with a default body** | ||
|
|
||
| ```rust | ||
| #[cfg(not(target_family = "wasm"))] | ||
| fn transfer_telemetry(&self) -> Option<Arc<TransferTelemetry>> { None } | ||
| ``` | ||
|
|
||
| **No existing implementor needs to change.** Only `RemoteClient` overrides it; the local, | ||
| in-memory, and simulation clients inherit `None` and report nothing. | ||
|
|
||
| Note the failure mode this creates: an override with a mistyped signature compiles cleanly and is | ||
| silently never called. `xet_data/tests/test_transfer_telemetry.rs` exists to catch that and should | ||
| not be deleted. | ||
|
|
||
| There is a second, subtler failure mode: the emit machinery can work perfectly while no production | ||
| caller ever reaches it. Tests that drive `FileDownloadSession` directly cannot see that, because | ||
| they call `finalize()` themselves. `xet_pkg/tests/test_download_telemetry.rs` exists to close the | ||
| gap — it goes through `XetFileDownloadGroup::finish_blocking()` and asserts on what the server | ||
| received. Keep new coverage at that altitude; a test that calls `finalize()` itself re-opens it. | ||
|
|
||
| ### Session behavior | ||
|
|
||
| - `FileUploadSession::finalize_impl` now delegates to a new private `finalize_inner` and reports on | ||
| both the success and error paths. Public signatures are unchanged. | ||
| - `FileDownloadSession::finalize` likewise, and reports a successful transfer. It keeps the debug | ||
| assertion that every item completed, so it must only be used on a clean-completion path. | ||
| - **New:** `FileDownloadSession::finalize_with(outcome, error_class)` finalizes while reporting an | ||
| explicit outcome, and makes no completeness claim. Use it for a session that ended badly, and for | ||
| one whose notion of "complete" belongs to the caller. `finalize` alone can only ever report `ok`, | ||
| so a download that failed has to go through this or the failure-rate signal is always zero. | ||
| - **Download groups now finalize their session.** `XetFileDownloadGroup::finish`/`finish_blocking` | ||
| and the legacy `data_client::download_async` finalize on both the success and error paths, the | ||
| latter classified via the new `XetError::telemetry_class()`. Previously nothing called | ||
| `FileDownloadSession::finalize`, so downloads through the Python bindings reported nothing at all. | ||
| - **`XetError` gained two variants: `RateLimited(String)` and `ServerError(String)`**, for HTTP 429 | ||
| and 5xx respectively. Additive, and the enum is already `#[non_exhaustive]`. | ||
| - **Python-visible behavior is unchanged**: both map to `PyConnectionError`, exactly as | ||
| `XetError::Network` did before. Only the message prefix differs (`Rate limited:` / | ||
| `Server error:` instead of `Network error:`). | ||
| - They exist because HTTP status did not survive the flattening into `XetError`. Every HTTP failure | ||
| became `Network`, so `telemetry_class()` could never return `rate_limited` or `server_error`, | ||
| while the upload path — which classifies from `DataError` and inspects `reqwest::Error::status()` | ||
| — reported both. A 429 therefore meant two different things depending on direction, defeating the | ||
| point of a shared `error_class` vocabulary. | ||
| - One gap is deliberately left: a 404 arriving as a `reqwest` status still classifies as `network`, | ||
| not `not_found`. Routing it correctly would change the Python exception type callers catch, which | ||
| is a user-visible change rather than a telemetry fix. Pinned by a test so it stays a decision. | ||
| - **New:** `XetDownloadStreamGroup::finish`/`finish_blocking` (and `finish()` plus context-manager | ||
| support on the Python class). Streams are consumed independently, so the group cannot detect | ||
| completion itself, and `finish` is how a caller states it explicitly. **Purely additive** - a | ||
| group that is never finished behaves exactly as before and still reports, so no existing caller | ||
| has to change. Note that `finish` closes the group: streams already handed out stay usable, but | ||
| opening a new one afterwards is an error. | ||
| - **New:** `XetDownloadStreamGroup::abort` (and `abort()` on the Python class) — the counterpart to | ||
| `finish` for a caller giving up rather than completing. Cancels every active stream. | ||
| - It emits **no** telemetry, matching `XetFileDownloadGroup::abort`. The session is left | ||
| unfinalized so its `Drop` derives the outcome from what actually transferred. | ||
| - `__exit__` now branches on `exc_type`, as the upload-commit and file-download-group context | ||
| managers already did: normal exit finishes, an exception aborts. Previously it called `finish` | ||
| unconditionally, which reports `ok` — so a `with` block that raised recorded the failed transfer | ||
| as a successful one, and `Drop` never got to correct it because `finish` had already set | ||
| `terminal_sent`. | ||
| - Reporting `error` on that path would have been wrong: the exception is often from the caller's | ||
| own code rather than the transfer, and classifying those as failures would inflate the very | ||
| failure rate this feature exists to measure. `cancelled` is reserved for a real user interrupt. | ||
| Deriving the outcome from progress avoids guessing. | ||
| - **Both sessions gained a `Drop` impl**, emitting a terminal summary when the session was never | ||
| finalized — the safety net for callers that abandon a session. It is deliberately *not* gated on | ||
| an ambient tokio runtime: the send is spawned on the `XetRuntime`'s own stored handle, so | ||
| requiring `Handle::try_current()` only served to disable the path for embedders that release the | ||
| last `Arc` from a foreign thread, which is exactly what the Python bindings do. | ||
| - An abandoned **upload** reports `aborted`: the commit never happened, so nothing was durably | ||
| transferred. | ||
| - An abandoned **download** reports `ok` or `dropped` depending on what actually transferred, via | ||
| the new `GroupProgress::all_items_complete()`. Reaching `Drop` only means nobody called | ||
| `finalize()`, which for downloads is the common case rather than a failure — `finish` is new and | ||
| existing embedders have not adopted it. Reporting all of those as `dropped` would make the | ||
| outcome field mean "probably fine" and leave a failure-rate dashboard nothing to measure, so | ||
| `dropped` is reserved for a genuinely incomplete transfer. | ||
| - `all_items_complete()` requires every item to have a **finalized size** as well as all of its | ||
| bytes delivered, and requires at least one item. The size check is not redundant: for an | ||
| open-ended stream range the size is discovered incrementally, so `bytes_completed` can equal | ||
| `total_bytes` mid-transfer whenever the consumer catches up to the prefetch frontier. A byte | ||
| comparison alone would report an abandoned stream as a success. | ||
| - New public accessors: `FileDownloadSession::client()`, and | ||
| `TestEnvironment::telemetry_docs()` under the `simulation` feature. | ||
| - New public helpers: `xet_data::telemetry::{classify_error, outcome_for_class}`, for callers that | ||
| need to produce an `(outcome, error_class)` pair themselves. | ||
|
|
||
| ### Simulation server | ||
|
|
||
| `POST /v1/telemetry` is now routed by the local test server, and received documents are readable | ||
| via `LocalServer::telemetry_docs()` / `LocalTestServer::telemetry_docs()`. | ||
|
|
||
| ### New dependencies | ||
|
|
||
| `chrono` and `uuid` were added to `xet-client`, gated to non-wasm targets. | ||
|
|
||
| ## Why | ||
|
|
||
| We had no client-side view of upload/download performance, so a throughput regression shipped in | ||
| an `hf-xet` release was undetectable. Server-side metrics cover CAS request latency but not | ||
| end-to-end client throughput, dedup effectiveness, or where a transfer's wall time goes. | ||
|
|
||
| ## Notes for downstream agents | ||
|
|
||
| - **The metric key set is a contract.** `xet_data/src/telemetry/payload.rs` is the source of truth. | ||
| Adding a key is safe; changing an existing key's JSON type is not — consumers assign a field | ||
| type on first sight and cannot change it in place, so a type change breaks ingestion for every | ||
| document carrying it. `test_upload_key_set_is_exact` and `test_numeric_types_stable` enforce this. | ||
| - **All `f64` values must go through the finite guard** in that module. `serde_json` renders NaN | ||
| and infinity as `null`, and a single such document poisons the field's type for a consumer. | ||
| - **No PII.** The payload carries no file names, paths, hashes, repository ids, or user ids; the | ||
| server derives identity from the request's JWT. | ||
| - A follow-up PR adds the generated schema (`telemetry/metrics.schema.json`) and a CI | ||
| compatibility gate. Until then the const key lists in the tests are the only thing pinning the | ||
| contract. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove mentioning of
HF_HUBenv vars as disabling telemetry is done programmatically.