Harden telemetry delivery and refine recipe metadata - #2632
Conversation
Replace PR #2443's accumulated history with one reviewable commit while retaining the finalized telemetry behavior, privacy boundary, and durable delivery guarantees. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20f671e7-7f05-4a33-b2e2-0f8d94d51585
There was a problem hiding this comment.
Pull request overview
This PR replaces Olive’s telemetry pipeline with a standard-library implementation that persists events to a per-user SQLite queue and uploads them in the background, while also refining “recipe” metadata emission and making telemetry opt-out consistent across entry points (CLI, Python API, Docker, workflows).
Changes:
- Replaced the OpenTelemetry-based logger/exporter with a durable SQLite offline store + background uploader, plus stricter privacy redaction for strings/config snapshots.
- Added/updated recipe-telemetry capture at workflow/CLI/Docker boundaries (including CI “recipe-only” behavior and forwarding opt-out/CI signals into containers).
- Expanded tests and updated documentation; removed
opentelemetry-sdkfrom runtime requirements.
Reviewed changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/workflows/test_workflow_run.py | Adds focused tests asserting recipe-result telemetry metadata, redaction behavior, hashing stability, and Docker-parent recipe semantics. |
| test/systems/docker/test_docker_system.py | Adds tests for CI/env forwarding and for disabling inner-container recipe/error telemetry in the workflow runner. |
| test/conftest.py | Ensures tests apply telemetry opt-out before singleton construction and shuts down telemetry at session end. |
| test/cli/test_cli.py | Adds coverage for CLI opt-out latching, telemetry lifecycle, and workflow-run recipe metadata injection. |
| test/cli/test_api.py | Verifies Python API surfaces latch disable_telemetry behavior. |
| test/cli/init/test_init_command.py | Adds init command telemetry action/error emission tests. |
| requirements.txt | Removes opentelemetry-sdk dependency from runtime requirements. |
| README.md | Updates telemetry blurb wording and formatting. |
| olive/workflows/run/run.py | Emits recipe-result and error telemetry around workflow execution, with CI-bounded shutdown behavior. |
| olive/telemetry/utils.py | Hardens telemetry base-dir resolution to require absolute per-user directories. |
| olive/telemetry/uploader.py | Introduces background uploader to drain SQLite queue with retry/split/ack/delete behavior and single-drainer locking. |
| olive/telemetry/telemetry.py | Reworks telemetry singleton to persist events to SQLite and upload via background thread; adds CI detection + full opt-out latching. |
| olive/telemetry/telemetry_redaction.py | Adds recursive redaction for secrets/paths/URLs/free-text and caps payload size. |
| olive/telemetry/telemetry_extensions.py | Updates action/error/recipe helpers, adds exception de-duplication marker, and formats tracebacks without source lines. |
| olive/telemetry/recipe_telemetry.py | Adds recipe metadata extraction, hashing with redaction, model/source classification, and config/package override snapshot sanitization. |
| olive/telemetry/process_lock.py | Adds cross-platform advisory single-holder lock for multi-process uploader coordination. |
| olive/telemetry/offline_store.py | Adds SQLite-backed durable FIFO event store with WAL/busy-timeout and permission hardening. |
| olive/telemetry/library/transport.py | Switches HTTP transport to urllib with worker-thread timeout bounding and retryability classification. |
| olive/telemetry/library/telemetry_logger.py | Deletes OpenTelemetry logger facade. |
| olive/telemetry/library/serialization.py | Tightens serialization (finite floats, deterministic ordering, key scrubbing, allow_nan=False). |
| olive/telemetry/library/retry.py | Deletes OpenTelemetry-era retry helper (superseded by uploader/store semantics). |
| olive/telemetry/library/options.py | Removes requests/http factory dependency and adjusts exporter options. |
| olive/telemetry/library/exporter.py | Deletes OpenTelemetry exporter implementation. |
| olive/telemetry/library/event_source.py | Deletes OpenTelemetry diagnostic event-source layer. |
| olive/telemetry/library/callback_manager.py | Deletes callback manager used by OpenTelemetry exporter path. |
| olive/telemetry/library/init.py | Updates package exports/docs to reflect stdlib-only building blocks. |
| olive/telemetry/deviceid/deviceid.py | Reworks device-id generation/publication, adds locking, and switches to hashed device-id reporting. |
| olive/telemetry/deviceid/_store.py | Hardens device-id storage (atomic write, permissions, size limits, registry type checks). |
| olive/telemetry/deviceid/init.py | Updates export to hashed-device-id function. |
| olive/telemetry/constants.py | Removes embedded connection-string constant file. |
| olive/telemetry/init.py | Exposes disable_telemetry alongside Telemetry and action. |
| olive/systems/docker/workflow_runner.py | Disables inner telemetry in container workflow runner and performs bounded shutdown flush. |
| olive/systems/docker/docker_system.py | Forwards CI + full telemetry opt-out into container env. |
| olive/cli/run.py | Captures explicit CLI overrides into recipe telemetry metadata and disables error telemetry for this path. |
| olive/cli/launcher.py | Latches opt-out before telemetry construction and ensures shutdown on all exit paths. |
| olive/cli/init/init.py | Adds telemetry options to init and wraps run with action telemetry. |
| olive/cli/base.py | Adds generated CLI recipe telemetry metadata to workflow execution path. |
| olive/cli/api.py | Latches opt-out when disable_telemetry is passed via Python API. |
| docs/Privacy.md | Updates privacy/telemetry documentation for opt-out semantics, CI behavior, SQLite durability, and redaction guarantees. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
olive/telemetry/uploader.py:223
- When an individual offline-store row is too large to fit into an empty PayloadBuilder (
builder.is_empty),drain_oncedrops the row via_finish_handled_rowsbut the returnedDrainResultreports it asdelivered=1. This makes thedeliveredcounter inaccurate for oversized/poison rows (they're deleted, not delivered), which can mislead any future accounting or diagnostics that useDrainResult.delivered.
if builder.is_empty:
return self._finish_handled_rows([row_id], deadline)
break
Rename DrainResult.delivered to handled because successful uploads, permanent drops, and acknowledged deletion recovery share the same completion path. Add an oversized single-row regression proving the row reports handled progress, is removed, and never reaches the transport. Verified at olive/telemetry/uploader.py:41 and test/test_telemetry.py:1181. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20f671e7-7f05-4a33-b2e2-0f8d94d51585
- telemetry.py: annotate _global_metadata as a class-level type hint (no assignment) so Pylint recognizes the attribute set per-instance in _new_unpublished_instance, resolving W0201 without changing singleton retry/disabled semantics. - telemetry_redaction.py: rewrite a two-part boolean comparison as the equivalent chained comparison (before_whitespace < separator < len(value)), resolving R1716. - telemetry_redaction.py: rename the Mapping loop variable in scrub_value_for_telemetry from key to child_key so it no longer shadows the function's own key parameter, resolving R1704. - uploader.py: keep the manual _mutation_lock.acquire()/release() in drain_once (a `with` block would double-release once on_send_admitted already released the lock mid-send), and add a precise rationale comment plus a narrow # pylint: disable=consider-using-with scoped to the single acquire call, resolving R1732. No behavior change. Verified: pylint targeted check clean on all four diagnostics, ruff check/format clean, diff-check clean, focused + full Windows telemetry/init suite (155 passed, 1 skipped, plus 2 known unrelated environment-timeout device-id tests), WSL telemetry suite (154 passed, 1 skipped), fresh material-code-review: No material findings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20f671e7-7f05-4a33-b2e2-0f8d94d51585
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated no new comments.
Suppressed comments (1)
olive/telemetry/telemetry_extensions.py:61
log_recipe_resultisn’t wrapped in asuppress(Exception)guard likelog_action/log_error. If telemetry initialization ever raises unexpectedly, this helper could propagate an exception into production call sites. Please make it best-effort/exception-safe for consistency with the rest of the telemetry helpers.
def log_recipe_result(
recipe_name: str,
success: bool,
metadata: Optional[dict[str, Any]] = None,
) -> None:
Copilot review flagged that log_recipe_result did not uphold the same "telemetry must never break the host application" boundary as log_action and log_error: those two wrap their entire body in `with suppress(Exception)`, but log_recipe_result called _get_logger() and telemetry.log(...) unguarded. The single current production caller (olive/workflows/run/run.py) already catches around it, but the helper itself should be safe for any future/internal caller too. - telemetry_extensions.py: wrap the _get_logger()/None-check/log path in log_recipe_result with `with suppress(Exception)`, matching log_action/log_error exactly. No behavior change for existing callers. - test_telemetry.py: extend test_public_helpers_never_propagate_failures to also exercise log_recipe_result under a raising _get_logger; add test_log_recipe_result_never_propagates_when_telemetry_log_raises, which patches _get_logger to return a logger whose .log() raises, and asserts log_recipe_result returns without propagating. Verified: focused pytest (4 recipe_result/public_helpers/log_error/ log_action tests) pass; full Windows test_telemetry.py + workflow_run.py suite: 177 passed, 1 skipped, plus the same 2 known unrelated environment-timeout device-id tests seen in prior rounds (slow `import olive` on this machine, not caused by this diff); WSL telemetry suite: 155 passed, 1 skipped; Ruff check/format clean; targeted Pylint on telemetry_extensions.py: 10.00/10; diff-check clean; fresh material-code-review: No material findings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20f671e7-7f05-4a33-b2e2-0f8d94d51585
Integrate the PR head while preserving recipe telemetry and multi-build execution. Resolve overlaps in olive/cli/run.py and olive/workflows/run/run.py. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use one bounded shutdown budget so durable queues avoid unnecessary exit-time network work while ephemeral Docker and CI queues still flush. Extend file-backed device IDs to all POSIX platforms and remove an unreachable duplicate workflow return. Files changed: - olive/telemetry/telemetry.py and callers - olive/telemetry/deviceid/deviceid.py - telemetry, Docker, and workflow tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Carry the newer shared test-option refactor into WorkflowRunCommand so --test_llama_path reaches discrepancy configuration and unused test-only options produce the same warnings as other CLI commands. Files changed: - olive/cli/run.py - test/cli/test_cli.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move SQLite mutations outside assert expressions, use one ctypes import form, and document the intentional best-effort exception paths so CodeQL can distinguish them from accidental silent failures. Files changed: - olive/telemetry/deviceid/deviceid.py - olive/telemetry/library/transport.py - olive/telemetry/telemetry.py - olive/telemetry/utils.py - test/test_telemetry.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Telemetry._build_payload can raise ValueError when serializing redacted config snapshots with non-finite floats (NaN/Inf), which would silently drop telemetry events unless handled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 40/41 changed files
- Comments generated: 1
- Review effort level: Lite
Preserve recipe telemetry when scrubbed snapshots contain non-finite floats by replacing only the invalid snapshot with the existing truncation marker. Remove the stale local Path import after merging current main, and evaluate SQLite deletion before asserting so optimized test runs retain the operation. Files changed: olive/cli/run.py, olive/telemetry/telemetry.py, test/test_telemetry.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The PR introduces a broad telemetry subsystem redesign (durable storage, uploader, locking, redaction, CI/Docker semantics) where correctness and operational safety merit final human review despite strong test coverage.
Review details
- Files reviewed: 40/41 changed files
- Comments generated: 0 new
- Review effort level: Lite
Restore the final newline removed by the web edit so EditorConfig validation succeeds. Files changed: docs/Privacy.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Describe your changes
This is a clean-history replacement for #2443. It supersedes that pull request for review purposes. The single replacement commit preserves the finalized reviewed tree exactly (
8403987295683c7eaf182777cb859858835abe18) while removing the accumulated development history.The preserved implementation:
For historical review context, see Hitesh's prior CHANGES_REQUESTED review commit
7427d751acf1293c28beba76b20d5df99fdfc36c.Validation:
test_capture_onnx_graph_integration) reproduces identically on pristineupstream/mainin the current Transformers environment.No material findings.Checklist before requesting a review
main.)lintrunner -aRelease-note summary: Olive telemetry now honors opt-out consistently and improves privacy-safe, durable delivery of Heartbeat and recipe metadata.
(Optional) Issue link
Supersedes #2443 with clean history; the original PR remains unchanged for now.