Skip to content

Harden telemetry delivery and refine recipe metadata#2443

Open
bmehta001 wants to merge 24 commits intomainfrom
bhamehta/fix-telemetry-deadlock
Open

Harden telemetry delivery and refine recipe metadata#2443
bmehta001 wants to merge 24 commits intomainfrom
bhamehta/fix-telemetry-deadlock

Conversation

@bmehta001
Copy link
Copy Markdown
Contributor

@bmehta001 bmehta001 commented May 5, 2026 •
edited
Loading

Same-repository replacement for #2441 so Azure CI runs with the normal microsoft/Olive PR secret context.

Summary

  • harden Olive telemetry delivery and cache behavior for non-CI runs, including Windows file locking, cache-path handling, and unreadable flush preservation
  • keep CI telemetry ephemeral: CI emits only OliveRecipe and does not register local cache callbacks
  • prevent Docker workflows from double-counting OliveRecipe by suppressing the inner container recipe event and forwarding CI detection into workflow containers
  • refine OliveRecipe metadata so target fields are emitted only for explicit targets, host fields are tracked separately, and recipe/config telemetry captures redacted explicit overrides
  • replace opaque package-config hashing with redacted package_config_overrides, while keeping package_config out of recipe_hash
  • make recipe source/hash metadata deterministic by avoiding filesystem-dependent model/resource classification
  • add or update focused telemetry, workflow, and Docker tests and refresh the privacy documentation

Why

We want telemetry to be reliable under cache contention and offline retry, while keeping CI runs temporal and recipe-only. We also want the recipe signal to be useful for analyzing explicit overrides without mixing host/target semantics, relying on opaque hashes, or depending on local filesystem state.

Validation

  • lintrunner f -a -m main
  • python -m compileall olive\\telemetry\\constants.py olive\\telemetry\\telemetry.py olive\\workflows\\run\\run.py olive\\systems\\docker\\docker_system.py test\\test_telemetry.py test\\workflows\\test_workflow_run.py test\\systems\\docker\\test_docker_system.py
  • pytest test\\test_telemetry.py test\\workflows\\test_workflow_run.py test\\systems\\docker\\test_docker_system.py -q
  • confirmed .azure_pipelines and .github do not set OLIVE_DISABLE_TELEMETRY, disable_telemetry, or --disable_telemetry

Copilot AI review requested due to automatic review settings May 5, 2026 02:51
Copilot AI reviewed May 5, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends Olive's telemetry pipeline so workflow runs emit a new OliveRecipe signal with redacted recipe metadata, while also tightening cache handling and documenting the updated telemetry behavior. It fits into the workflow, CLI, and telemetry layers that capture how Olive runs recipes across local and CI environments.

Changes:

  • Adds recipe-result telemetry from workflow execution and CLI entry points, including recipe hashes, model/source metadata, host/target metadata, and redacted config/package override snapshots.
  • Reworks telemetry delivery details, including cache-path overrides, JSON-line cache storage, Windows file locking, unreadable flush preservation, and CI recipe-only filtering.
  • Adds focused workflow and telemetry tests, plus privacy-documentation updates.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
test/workflows/test_workflow_run.py Adds coverage for recipe-result metadata emitted by workflow runs.
test/test_telemetry.py Adds tests for cache-path overrides, CI filtering, unreadable flush handling, and Windows file locking.
olive/workflows/run/run.py Builds and emits recipe telemetry metadata during workflow execution.
olive/telemetry/utils.py Reworks cross-platform exclusive file locking for telemetry cache access.
olive/telemetry/telemetry.py Adds OliveRecipe, CI filtering, and cache/flush behavior changes.
olive/telemetry/telemetry_extensions.py Adds a helper for logging recipe results.
olive/telemetry/library/telemetry_logger.py Adds locking around singleton logger setup and configurable service name.
olive/telemetry/library/options.py Extends exporter options with service_name.
olive/telemetry/constants.py Updates the encoded telemetry connection string.
olive/cli/run.py Passes workflow-run recipe metadata into telemetry.
olive/cli/base.py Passes generated-CLI recipe metadata from higher-level commands.
docs/Privacy.md Updates the privacy text for the new recipe telemetry behavior.

Comment thread olive/workflows/run/run.py Outdated
Comment thread olive/telemetry/telemetry.py
Comment thread olive/telemetry/telemetry.py
Comment thread olive/workflows/run/run.py
Comment thread olive/workflows/run/run.py Outdated
Comment thread docs/Privacy.md Outdated
@bmehta001 bmehta001 self-assigned this May 5, 2026
@bmehta001
Copy link
Copy Markdown
Contributor Author

bmehta001 commented May 5, 2026

/AzurePipelines help

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented May 5, 2026

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@bmehta001
Copy link
Copy Markdown
Contributor Author

bmehta001 commented May 5, 2026

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented May 5, 2026

Azure Pipelines successfully started running 1 pipeline(s).

bmehta001 and others added 21 commits May 8, 2026 22:35
@bmehta001
Consolidate TelemetryCacheHandler to single lock
fad52d8 
Replace separate _lock and _callback_condition with a single
_condition (threading.Condition) that protects all shared state:
_shutdown, _is_flushing, _callbacks_item_count, _events_logged.

The two-lock design had a lock order inversion: on_payload_transmitted
acquired _lock then _callback_condition, while wait_for_callbacks
acquired _callback_condition then _lock (via is_flushing). This could
deadlock under concurrent flush + callback scenarios.

Using one lock eliminates the ordering issue entirely and simplifies
the code — the on_payload_transmitted callback no longer needs nested
lock acquisition.

Files changed:
- olive/telemetry/telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Fix concurrency issues in telemetry
fe680ac 
wait_for_callbacks: Hold _condition lock continuously from condition
check through wait() to prevent missing notifications. Previously the
lock was released between checking and waiting, allowing notify_all()
to fire in the gap.

TelemetryLogger: Add RLock to __new__ and get_default_logger to
prevent race conditions when multiple threads create the singleton
simultaneously. Uses RLock because get_default_logger calls __new__
which both need the same lock.

Files changed:
- olive/telemetry/telemetry.py
- olive/telemetry/library/telemetry_logger.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Simplify telemetry cache and singleton patterns
8aa3d3d 
1. Remove base64 encoding from cache: write raw JSON lines instead.
   The base64 layer added 33% size overhead and prevented human
   inspection during debugging with no security benefit (cache dir
   is user-owned). Cache file is now directly readable.

2. Simplify cache flush: remove the .flush file rename dance
   (atomic rename, restore on failure, stale file cleanup). For ~5
   events/session, simple lock-read-send-delete is sufficient. On
   failure the cache file persists for next retry.

3. Simplify Telemetry singleton: remove double-checked locking in
   __new__. The lock is cheap and called once at startup; the outer
   check saved a lock acquisition but added complexity.

Net: -83 lines.

Files changed:
- olive/telemetry/telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Fix callback double-counting and multi-process cache overwrite
166487c 
Fix 1: Remove duplicate callback count increment in
on_payload_transmitted when _is_flushing is true. The count was
incremented both in the early-return path and in the finally block,
causing wait_for_callbacks to think events completed before they
actually did. Now only the finally block increments (which always
runs, even on return).

Fix 2: Replace flush_path.replace(cache_path) with new
_restore_flush_file() method that appends flush entries into the
cache file instead of overwriting it. This prevents losing events
written by another process while a flush was in progress.

Files changed:
- olive/telemetry/telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Track recipe telemetry in CI
a6ff131 
Emit a single OliveRecipe event per workflow so recipe usage and failures can be measured without double-counting nested action failures. Keep CI in recipe-only mode, make Olive app naming explicit, update the telemetry ingestion key, and harden the cache/locking path that the new telemetry depends on.

Files changed:
- docs/Privacy.md
- olive/cli/base.py
- olive/cli/run.py
- olive/telemetry/constants.py
- olive/telemetry/library/options.py
- olive/telemetry/library/telemetry_logger.py
- olive/telemetry/telemetry.py
- olive/telemetry/telemetry_extensions.py
- olive/telemetry/utils.py
- olive/workflows/run/run.py
- test/test_telemetry.py
- test/workflows/test_workflow_run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Remove avoidable telemetry lint suppressions
15a5c31 
Replace optional runtime imports with importlib-based lookups so the recent telemetry changes stay lint-clean without adding new noqa markers. Keep the focused telemetry tests import-sorted and ready for CI.

Files changed:
- olive/cli/base.py
- olive/workflows/run/run.py
- test/test_telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Move service name handling into telemetry logger
eb524c7 
Keep exporter options focused on transport/export concerns and move service.name defaults into the logger/resource layer where they belong. This keeps Olive's explicit app name override separate from the shared logger fallback and removes unnecessary plumbing.

Files changed:
- olive/telemetry/library/options.py
- olive/telemetry/library/telemetry_logger.py
- test/test_telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Scope service-name cleanup to Olive usage
862a9a1 
Keep Olive''s explicit service-name override in the logger path, but restore the previous shared-library fallback and compatibility behavior so this cleanup does not broaden unrelated API or default changes.

Files changed:
- olive/telemetry/library/options.py
- olive/telemetry/library/telemetry_logger.py
- test/test_telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Revert unnecessary non-telemetry branch changes
86d4d02 
Trim the branch back to telemetry behavior and the minimal plumbing needed to support it. Restore the original local-import patterns in CLI and workflow code, keep only targeted lint suppressions where those restored lines are required, and simplify the telemetry logger app-name plumbing without changing the feature behavior.

Files changed:
- olive/cli/base.py
- olive/cli/run.py
- olive/telemetry/library/telemetry_logger.py
- olive/workflows/run/run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Address PR review feedback on telemetry changes
99f0e17 
Fix the correctness and security issues raised on PR #2441 by handling empty telemetry cache-dir overrides safely, preserving non-empty unreadable flush files instead of deleting them, restoring the legacy .json.flush naming pattern, handling non-pathlike recipe config inputs without masking the original error, and cleaning up the Windows ctypes import pattern for CodeQL.

Files changed:
- olive/telemetry/telemetry.py
- olive/telemetry/utils.py
- olive/workflows/run/run.py
- test/test_telemetry.py
- test/workflows/test_workflow_run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Remove low-value service-name telemetry test
788e91c 
Drop the explicit service.name wiring test from test/test_telemetry.py since it is mostly implementation-detail coverage and does not protect the higher-value telemetry behavior changes on this branch.

Files changed:
- test/test_telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Address remaining GitHub Advanced Security comments
06f1252 
Resolve the remaining github-advanced-security findings by removing unused PLC0415 noqa markers, keeping the optional-import behavior via minimal import cleanup, and updating the telemetry tests to satisfy the protected-access and consider-using-with lint comments without changing the tested behavior.

Files changed:
- olive/cli/base.py
- olive/cli/run.py
- olive/workflows/run/run.py
- test/test_telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Simplify telemetry utils responsibilities
42dbce5 
Remove dead base64 cache helpers from olive/telemetry/utils.py and move exception formatting next to the telemetry extension code that actually uses it. Keep the Win32 locking and cache-dir logic intact while reducing unrelated utility clutter.

Files changed:
- olive/telemetry/utils.py
- olive/telemetry/telemetry_extensions.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Store CI detection result once in telemetry init
d948cd9 
Compute the CI environment flag once during Telemetry initialization and reuse it for recipe-only gating and heartbeat suppression instead of calling the check twice back-to-back.

Files changed:
- olive/telemetry/telemetry.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Refine recipe telemetry semantics and config tracking
1ff609a 
Keep target telemetry fields only for explicitly configured targets, add separate host fields, remove the ambiguous input model name hash, and add redacted config-overrides plus package-config hash metadata so recipe telemetry can show which overrides users actually provide without folding environment-specific package config into recipe_hash.

Files changed:
- docs/Privacy.md
- olive/telemetry/telemetry.py
- olive/workflows/run/run.py
- test/workflows/test_workflow_run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Replace package config hash with override values
8987669 
Log redacted package-config overrides instead of an opaque hash so Olive telemetry captures the specific package settings users changed, while still excluding package_config from recipe_hash and avoiding raw module-path leakage.

Files changed:
- docs/Privacy.md
- olive/telemetry/telemetry.py
- olive/workflows/run/run.py
- test/workflows/test_workflow_run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Guard Azure CI secret-dependent login steps
ff60650 
Skip Hugging Face and Docker logins when PR secrets are unavailable or unresolved so Azure unit-test jobs do not fail before tests start on fork-style runs, while still preserving normal login behavior when valid credentials are present.

Files changed:
- .azure_pipelines/job_templates/build-docker-image-template.yaml
- .azure_pipelines/job_templates/huggingface-login-template.yaml
- .azure_pipelines/job_templates/olive-test-linux-gpu-template.yaml
- .azure_pipelines/scripts/run_test.sh

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Revert Azure CI secret login guards
1cae73f 
Revert the Azure pipeline login guard changes because the pipeline behavior should match main and those changes are not necessary for the telemetry PR.

Files changed:
- .azure_pipelines/job_templates/build-docker-image-template.yaml
- .azure_pipelines/job_templates/huggingface-login-template.yaml
- .azure_pipelines/job_templates/olive-test-linux-gpu-template.yaml
- .azure_pipelines/scripts/run_test.sh

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Address telemetry review comments
14130d4 
Avoid duplicate OliveRecipe telemetry from Docker workflows by suppressing inner workflow recipe events and forwarding CI detection into workflow containers. Keep CI telemetry ephemeral by skipping cache setup, and make recipe metadata stable by avoiding filesystem-sensitive model/resource classification.

Files changed:

- docs/Privacy.md

- olive/systems/docker/docker_system.py

- olive/telemetry/constants.py

- olive/telemetry/telemetry.py

- olive/workflows/run/run.py

- test/systems/docker/test_docker_system.py

- test/test_telemetry.py

- test/workflows/test_workflow_run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Fix telemetry pipeline test regressions
d9247e2 
Keep CLI workflow imports patchable so existing CLI tests and command mocks still intercept workflow execution. Update CLI test expectations for recipe telemetry metadata, and make the CI-sensitive workflow telemetry assertion deterministic.

Files changed:

- olive/cli/base.py

- olive/cli/run.py

- test/cli/test_cli.py

- test/workflows/test_workflow_run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Log workflow exceptions as error telemetry
befbb08 
Keep OliveRecipe focused on recipe outcome metadata and use the existing log_error path for workflow exceptions. This avoids duplicating exception fields on recipe events while preserving detailed formatted exception messages in error telemetry.

Files changed:

- olive/telemetry/telemetry_extensions.py

- olive/telemetry/telemetry.py

- olive/workflows/run/run.py

- test/workflows/test_workflow_run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001 bmehta001 force-pushed the bhamehta/fix-telemetry-deadlock branch from e47e0ce to befbb08 Compare May 9, 2026 03:47
bmehta001 and others added 3 commits May 8, 2026 23:13
@bmehta001
Reduce CLI import churn
c90e653 
Keep optional and workflow-heavy imports lazy so generated CLI commands preserve existing import behavior while still reporting recipe telemetry.

Files changed:
- olive/cli/base.py
- olive/cli/run.py
- olive/workflows/run/run.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Simplify Docker recipe telemetry suppression
51d08fd 
Use an explicit run() parameter for the inner Docker workflow runner instead of an environment variable so only the parent workflow emits the recipe result event.

Files changed:
- olive/workflows/run/run.py
- olive/systems/docker/workflow_runner.py
- olive/systems/docker/docker_system.py
- olive/telemetry/constants.py
- test/workflows/test_workflow_run.py
- test/systems/docker/test_docker_system.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bmehta001
Keep platform imports local
38f118c 
Restore local imports for Docker token lookup and telemetry file locking to avoid unnecessary module-level import churn.

Files changed:
- olive/systems/docker/docker_system.py
- olive/telemetry/utils.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@bmehta001 bmehta001

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bmehta001