Skip to content

Implement SDK v2 telemetry with privacy controls - #953

Open
bmehta001 wants to merge 12 commits into
mainfrom
bhamehta/flcore/1ds-telemetry-core-minimal
Open

Implement SDK v2 telemetry with privacy controls#953
bmehta001 wants to merge 12 commits into
mainfrom
bhamehta/flcore/1ds-telemetry-core-minimal

Conversation

@bmehta001

@bmehta001 bmehta001 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Implement the SDK v2 telemetry system end to end, including telemetry contracts, event collection, 1DS delivery, privacy controls, cross-SDK configuration, and targeted sampling.

What changed

  • Added typed telemetry contracts and event collection for actions, sessions, model usage, downloads, catalog fetches, execution providers, hardware, audio usage, exceptions, and startup process information.
  • Added the 1DS backend with local logging, lifecycle cleanup, cache isolation, device identity handling, and upload gating.
  • Wired DisableNonessentialTelemetry through the C++, C#, JavaScript, and Python SDKs with authoritative opt-out precedence.
  • Kept essential ProcessInfo telemetry available for client opt-out, while hard-suppressing all telemetry for CI/tests and ORT_TELEMETRY_DISABLED.
  • Added privacy-safe redaction for paths and error content, and suppressed device-ID status when telemetry is hard-disabled.
  • Added deterministic action sampling: OpenAIAudioTranscribe is sampled at 2%; other actions use the default rate. Reported popSample values match the effective sampling rate.
  • Removed locale collection from ProcessInfo telemetry.
  • Added regression coverage for opt-out precedence, hard suppression, cache isolation, redaction, sampling, correlation IDs, and telemetry formatting.

Validation

  • C++ RelWithDebInfo build completed successfully.
  • Focused C++ telemetry tests: 19 passed.
  • Python configuration tests and JavaScript native addon validation were completed in earlier iterations.

bmehta001 and others added 3 commits August 3, 2026 21:57
Keep the first telemetry slice focused on enable/disable controls, native 1DS initialization, startup ProcessInfo, and redacted Error logging.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 297bc539-5f7f-4a70-9411-48e91a5bf532
…ling

- Modified Manager::Initialize to normalize DisableNonessentialTelemetry from both
  config field and additional_options, ensuring opt-out from public SDKs reaches
  native initialization (C#, JS, Python SetDisableNonessentialTelemetry forwarded
  via additional_options were being ignored)

- Added action-specific sampling support:
  - Added kCoreAudioTranscribeSampleRatePercent = 2.0 constant
  - Added SampleRateForAction() to map action names to sample rates
  - Updated RecordAction() to pass action-specific rate to ShouldSampleEvent()
  - OpenAIAudioTranscribe (action 203) now sampled at 2%, all others at 100%

- Added test SamplesCoreAudioTranscribeAtTwoPercent to verify behavior

Fixes cross-language telemetry opt-out parity and implements targeted sampling
for high-volume action as requested in security/architecture review.

All 18 telemetry unit tests pass.

Files changed:
- sdk_v2/cpp/src/manager.cc: opt-out normalization at native init
- sdk_v2/cpp/src/telemetry/telemetry_sampling.h: rate constants and mapper
- sdk_v2/cpp/src/telemetry/one_ds_telemetry.cc: action-specific rate dispatch
- sdk_v2/cpp/test/internal_api/telemetry_test.cc: new sampling test

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5a9051ad-9c32-4dc3-bd66-d33d416145a4
Copilot AI balanced review requested due to automatic review settings August 5, 2026 22:22
@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
foundry-local Ready Ready Preview Aug 6, 2026 9:27pm

Request Review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Wires telemetry opt-out across SDKs and adds 1DS telemetry sampling infrastructure to the C++ core.

Changes:

  • Exposes non-essential telemetry opt-out in C++, C#, Python, and JavaScript.
  • Adds 1DS metadata, device identity, redaction, lifecycle, and 2% audio-action sampling.
  • Expands telemetry tests and build dependencies.

Reviewed changes

Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
sdk_v2/python/src/foundry_local_sdk/configuration.py Adds Python opt-out wiring.
sdk_v2/js/src/detail/native.ts Extends native manager options.
sdk_v2/js/src/configuration.ts Exposes JavaScript opt-out.
sdk_v2/js/native/src/manager.cc Maps JavaScript telemetry settings.
sdk_v2/cs/src/FoundryLocalManager.cs Applies C# opt-out setting.
sdk_v2/cs/src/Configuration.cs Exposes C# opt-out property.
sdk_v2/cpp/vcpkg.json Adds telemetry feature dependencies.
sdk_v2/cpp/test/internal_api/web_service_test.cc Updates telemetry test fixtures.
sdk_v2/cpp/test/internal_api/test_helpers.h Includes telemetry test support.
sdk_v2/cpp/test/internal_api/telemetry_test.cc Expands telemetry coverage.
sdk_v2/cpp/test/internal_api/session_manager_test.cc Updates telemetry fixture.
sdk_v2/cpp/test/internal_api/null_telemetry.h Removes obsolete test fake.
sdk_v2/cpp/test/internal_api/chat/chat_session_test.cc Updates chat telemetry fixture.
sdk_v2/cpp/test/internal_api/audio/audio_session_test.cc Updates audio telemetry fixtures.
sdk_v2/cpp/src/util/sha256.h Declares string hashing.
sdk_v2/cpp/src/util/sha256.cc Implements cross-platform hashing.
sdk_v2/cpp/src/telemetry/telemetry.h Expands telemetry contracts.
sdk_v2/cpp/src/telemetry/telemetry.cc Maps actions and statuses.
sdk_v2/cpp/src/telemetry/telemetry_sampling.h Implements deterministic sampling.
sdk_v2/cpp/src/telemetry/telemetry_redaction.h Adds telemetry redaction.
sdk_v2/cpp/src/telemetry/telemetry_metadata.h Defines process metadata.
sdk_v2/cpp/src/telemetry/telemetry_metadata.cc Collects platform metadata.
sdk_v2/cpp/src/telemetry/telemetry_logger.h Expands logger interface.
sdk_v2/cpp/src/telemetry/telemetry_logger.cc Formats telemetry events.
sdk_v2/cpp/src/telemetry/telemetry_environment.h Declares runtime gating.
sdk_v2/cpp/src/telemetry/telemetry_environment.cc Implements CI/env gating.
sdk_v2/cpp/src/telemetry/telemetry_action_tracker.h Adds invocation contexts.
sdk_v2/cpp/src/telemetry/telemetry_action_tracker.cc Improves action tracking.
sdk_v2/cpp/src/telemetry/one_ds_tenant_token.h.in Templates the tenant token.
sdk_v2/cpp/src/telemetry/one_ds_telemetry.h Declares the 1DS backend.
sdk_v2/cpp/src/telemetry/one_ds_telemetry.cc Implements 1DS telemetry.
sdk_v2/cpp/src/telemetry/invocation_context.h Defines correlation context.
sdk_v2/cpp/src/telemetry/invocation_context.cc Implements GUID/user-agent helpers.
sdk_v2/cpp/src/telemetry/device_id.h Defines device identity storage.
sdk_v2/cpp/src/telemetry/device_id.cc Implements platform device IDs.
sdk_v2/cpp/src/telemetry/android_telemetry_bridge.cc Adds Android readiness bridge.
sdk_v2/cpp/src/manager.cc Initializes 1DS and opt-out behavior.
sdk_v2/cpp/src/configuration.h Adds native opt-out field.
sdk_v2/cpp/ports/cpp-client-telemetry/vcpkg.json Defines the 1DS overlay port.
sdk_v2/cpp/ports/cpp-client-telemetry/portfile.cmake Builds the 1DS dependency.
sdk_v2/cpp/include/foundry_local/foundry_local_cpp.inline.h Implements C++ opt-out setter.
sdk_v2/cpp/include/foundry_local/foundry_local_cpp.h Declares C++ opt-out API.
sdk_v2/cpp/CMakeLists.txt Integrates telemetry sources and linking.
sdk_v2/cpp/build.py Enables overlay ports and features.
Suppressed comments (1)

sdk_v2/cs/src/FoundryLocalManager.cs:253

  • The user dictionary is copied after the dedicated opt-out flag, so an entry of DisableNonessentialTelemetry=false silently defeats DisableNonessentialTelemetry = true. Make the public opt-out monotonic by applying it after AdditionalSettings; otherwise this API can send non-essential telemetry despite the caller opting out.

EventProperties ev(name);
ev.SetPriority(EventPriority::EventPriority_Normal);
ev.SetPolicyBitFlags(kCriticalData);
ev.SetPopsample(TelemetryInternal::kTelemetrySampleRatePercent);
Comment thread sdk_v2/cpp/src/telemetry/one_ds_telemetry.cc Outdated
Comment thread sdk_v2/python/src/foundry_local_sdk/configuration.py
Comment on lines +273 to +279
try {
impl_->log_manager->Flush();
impl_->log_manager->FlushAndTeardown();
LogManagerProvider::Release(impl_->config);
} catch (...) {
// Best-effort: never throw from a destructor.
}
- Preserve the selected action sample rate in the emitted popSample field.
- Make 1DS manager cleanup independent so Release runs after flush failures.
- Mark the manager acquired immediately after CreateLogManager succeeds.
- Keep dedicated telemetry opt-out settings authoritative over user overrides in C# and Python.
- Replace the stale NullTelemetry audio test fixture after merging main.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5a9051ad-9c32-4dc3-bd66-d33d416145a4

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.

Suppressed comments (7)

sdk_v2/cpp/src/manager.cc:314

  • This added line exceeds the repository's 120-character C++ line limit.
    logger_->Log(LogLevel::Warning, fmt::format("telemetry ProcessInfo failed during Manager initialization: {}", ex.what()));

sdk_v2/cpp/src/telemetry/one_ds_telemetry.cc:181

  • This gate suppresses CI only; local foundry_local_tests, sdk_integration_tests, and cache_only_tests runs do not set this or ORT_TELEMETRY_DISABLED, so they initialize the production 1DS client and can upload test events. That contradicts the class contract that unit-test processes are fully suppressed and pollutes operational telemetry. Add an explicit test-process/build gate or set the opt-out environment for every test registration before initialization.
  if (TelemetryEnvironment::IsCiEnvironment()) {
    logger_.Log(LogLevel::Information,
                "[Telemetry] CI environment detected; 1DS upload disabled (events still logged locally)");
    return;

sdk_v2/js/native/src/manager.cc:218

  • An explicit disableNonessentialTelemetry: false overwrites additionalSettings.DisableNonessentialTelemetry = "true", re-enabling uploads despite the legacy opt-out. The core and the Python/C# bindings treat either opt-out source as authoritative, so only inject this key when the typed flag is true.
    sdk_v2/cpp/src/telemetry/telemetry.h:213
  • This compatibility overload emits an empty correlation ID. Manager::Create still calls this overload, so its CoreInitialize event cannot satisfy the new correlation contract even though the other compatibility overloads create a direct context. Construct a direct context here and then apply the requested indirect flag.
  void RecordAction(Action action, ActionStatus status, const std::string& user_agent,
                    bool indirect, int64_t duration_ms) {
    RecordAction(action, status, InvocationContext{user_agent, {}, indirect}, duration_ms);
  }

sdk_v2/cpp/src/manager.cc:308

  • The tests exercise OneDsTelemetry(true) directly, but none verifies this normalization point. Add coverage proving both the native configuration field and additional_options["DisableNonessentialTelemetry"] suppress uploads, including accepted truthy values, so the primary wiring fixed by this PR cannot regress.

This issue also appears on line 314 of the same file.

  const bool disable_nonessential_telemetry =
      config_.disable_nonessential_telemetry ||
      IsAdditionalOptionEnabled(config_, "DisableNonessentialTelemetry");

sdk_v2/cs/src/FoundryLocalManager.cs:260

  • The C# manager tests do not cover this new telemetry-consent wiring. Add a configuration/native handoff test that verifies the default, enabled state, and precedence against a conflicting AdditionalSettings entry.
    sdk_v2/python/src/foundry_local_sdk/configuration.py:249
  • The Python configuration suite covers additional-setting serialization and native construction, but no test exercises this new opt-out branch or its precedence over a conflicting additional setting. Add unit/native-configuration coverage for the default, enabled, and conflict cases.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.

Suppressed comments (3)

sdk_v2/cpp/test/internal_api/telemetry_test.cc:150

  • This does not exercise the opt-out wiring: it constructs OneDsTelemetry directly, and in CI the constructor exits because CI/GITHUB_ACTIONS is set, so IsUploadEnabled() is false even if the configuration flag is ignored. Add a test that drives DisableNonessentialTelemetry through Manager/additional_options and can distinguish configuration suppression from CI, token, or initialization failure; it should also verify that nonessential events are blocked while ProcessInfo remains eligible.
    sdk_v2/cpp/test/internal_api/telemetry_test.cc:361
  • This only verifies the lookup constant, not the client-side sampling behavior used by RecordAction. A broken threshold/hash path could still retain 0% or 100% while this passes. Exercise ShouldSampleTelemetryEvent at the returned 2% rate over deterministic keys (or fixed sampled/rejected vectors) and assert the expected retained proportion/decisions.
    sdk_v2/cpp/src/manager.cc:314
  • This added line exceeds the repository's 120-character C++ line limit. Wrap the logging call so the changed code conforms to the enforced project style.
    logger_->Log(LogLevel::Warning, fmt::format("telemetry ProcessInfo failed during Manager initialization: {}", ex.what()));

- Disable 1DS telemetry in both C++ test executables before test initialization.
- Preserve correlation IDs in the compatibility action overload.
- Keep JavaScript additional-setting opt-outs authoritative when the typed flag is false.
- Wrap the long Manager telemetry warning and add correlation regression coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5a9051ad-9c32-4dc3-bd66-d33d416145a4

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

sdk_v2/cpp/test/internal_api/telemetry_test.cc:152

  • This test cannot exercise the configuration opt-out: test_main.cc sets ORT_TELEMETRY_DISABLED=1 for the entire process, so the constructor exits at the environment gate and IsUploadEnabled() is false even if this argument is changed to false. Add an observable test for the configuration gate or run this case in an isolated process without the global suppression.

Comment on lines +219 to +221
if (const auto cache_dir = TelemetryDeviceId::EnsureCacheDirectory(); !cache_dir.empty()) {
config[CFG_STR_CACHE_FILE_PATH] = (cache_dir / "foundry-local.db").string();
}
Use a separate 1DS cache file when non-essential telemetry is disabled so
queued non-essential events from normal telemetry sessions cannot be flushed
by an opted-out process. ProcessInfo remains eligible for essential upload.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5a9051ad-9c32-4dc3-bd66-d33d416145a4

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

sdk_v2/cpp/test/internal_api/telemetry_test.cc:152

  • This test cannot verify the configuration switch because test_main.cc sets ORT_TELEMETRY_DISABLED=1 for the entire process. The constructor therefore returns at the environment gate before it evaluates disable_nonessential_telemetry, and the assertion would still pass if the configuration argument were ignored. Add a testable state/initialization seam that distinguishes configuration suppression from the hard test/CI gate.
    sdk_v2/cpp/src/telemetry/telemetry.h:66
  • kClientError is documented as the status for HTTP 4xx, but the service handlers return ErrorResponse(...) directly for validation/not-found paths without setting the tracker status or throwing. Since ActionTracker defaults to kFailure, every such client error is emitted as an internal service failure. Set kClientError before 4xx returns, preferably through a centralized response/status mapping so all handlers remain consistent.
  kFailure = 0,    // Internal failure while executing valid work
  kSuccess,
  kInvalid,
  kSkipped,
  kClientError,    // Rejected due to invalid client input (maps to HTTP 4xx) — not a service fault

sdk_v2/cpp/src/manager.cc:312

  • The environment opt-out and CI gates are checked only inside OneDsTelemetry, but this call still asks BuildProcessInfo for the device-ID status whenever the configuration flag is false. GetStatusString() initializes and persists the telemetry device ID, so ORT_TELEMETRY_DISABLED=1 (and CI, including the new test main) still writes telemetry state even though upload is hard-disabled. Include the environment/CI gates when deciding include_device_id_status, or skip this metadata construction when 1DS was not initialized.
    telemetry_->RecordProcessInfo(
        BuildProcessInfo(BuildTelemetryMetadata(config_.app_name), !disable_nonessential_telemetry));

sdk_v2/cpp/src/telemetry/one_ds_telemetry.cc:175

  • The PR description presents this as opt-out wiring plus one sampling change, but the diff also introduces the 1DS backend, persistent device identity/cache handling, process metadata collection, new event schemas/statuses, and build/dependency changes across platforms. That is materially broader and privacy/operationally significant. Please update the description and validation matrix to cover the full telemetry rollout, or split the backend rollout from the focused wiring/sampling fix.
OneDsTelemetry::OneDsTelemetry(const std::string& app_name,
                               ILogger& logger,
                               bool disable_nonessential_telemetry)
    : local_log_(app_name, logger),

Comment on lines +46 to +49
if (c == '/') {
if (i == 0) {
return i;
}
Treat POSIX slash paths after whitespace or quotes as path anchors even when
there is only one component, preventing strings such as ailed at /secret
from reaching telemetry unchanged. Add regression coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5a9051ad-9c32-4dc3-bd66-d33d416145a4

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.

Suppressed comments (2)

sdk_v2/cpp/src/manager.cc:312

  • Hard telemetry suppression does not prevent device-ID creation here. When ORT_TELEMETRY_DISABLED is truthy or CI is detected, OneDsTelemetry returns without initializing 1DS, but this call still passes true to BuildProcessInfo whenever the configuration flag is false; BuildProcessInfo then initializes TelemetryDeviceId, writing the registry/file identifier even though telemetry is hard-disabled. Include the environment/CI suppression state when deciding include_device_id_status (or skip ProcessInfo construction entirely for hard suppression).
    telemetry_->RecordProcessInfo(
        BuildProcessInfo(BuildTelemetryMetadata(config_.app_name), !disable_nonessential_telemetry));

sdk_v2/cpp/test/internal_api/telemetry_test.cc:152

  • This assertion cannot verify the configuration opt-out: the new test_main.cc sets ORT_TELEMETRY_DISABLED=1 for the entire test process, so the constructor exits through the environment suppression branch before it evaluates disable_nonessential_telemetry. The test therefore also passes if the configuration parameter is ignored. Exercise the configuration branch with an injectable 1DS backend/gating decision or add a test seam that avoids real uploads.

Do not include device-ID status in ProcessInfo construction when CI or
ORT_TELEMETRY_DISABLED hard-disables telemetry, preventing unnecessary
persistent identifier creation in fully opted-out processes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5a9051ad-9c32-4dc3-bd66-d33d416145a4

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

sdk_v2/cpp/src/telemetry/telemetry_redaction.h:72

  • The two-segment threshold leaves common relative paths such as models/alice.onnx unchanged, so exception and catalog error telemetry can still upload path/user data. Extend relative-path detection to redact one-separator path tokens while preserving intended non-path values such as the tested 3/4 fraction.
      if (segments >= 2) {

sdk_v2/cpp/test/internal_api/telemetry_test.cc:375

  • This only checks the action-to-rate lookup; it never exercises the fractional sampling decision or verifies that retained and rejected keys exist at 2%. The test would still pass if the threshold/hash logic mishandled fractional rates. Add deterministic keys on both sides of the 2% threshold (and ideally verify RecordAction uses that rate).
    sdk_v2/cpp/src/telemetry/telemetry.h:66
  • This status is never emitted in production: the only non-test assignment is ActionStatusFromException, while HTTP handlers return 4xx ErrorResponses without throwing and their trackers retain the default kFailure. As a result, invalid client requests are reported as service failures despite the documented mapping. Set kClientError on 4xx exits, preferably in a centralized response/tracker path.
  kClientError,    // Rejected due to invalid client input (maps to HTTP 4xx) — not a service fault

sdk_v2/cpp/test/internal_api/telemetry_test.cc:152

  • This test cannot verify the configuration opt-out: test_main.cc sets ORT_TELEMETRY_DISABLED=1 for the whole process, so the constructor returns through the hard-disable path and IsUploadEnabled() is false even if disable_nonessential_telemetry is ignored. Exercise the configuration gate independently of the global test suppression (for example via a pure gating helper or an observable suppression reason).

This issue also appears on line 373 of the same file.

Comment on lines +33 to +36
/// This action's context, with its correlation id resolved. Use to derive a
/// child context (Context().AsIndirect()) for any caused-by action so all
/// events from one operation share a correlation id.
const InvocationContext& Context() const { return context_; }
Redact relative path tokens with file-like components while preserving
fractions and ordinary slash-separated text. Expand the 2% sampling test to
exercise both retained and rejected deterministic keys.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5a9051ad-9c32-4dc3-bd66-d33d416145a4

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.

Suppressed comments (1)

sdk_v2/cpp/test/internal_api/telemetry_test.cc:152

  • This test never exercises the configuration opt-out path: test_main.cc sets ORT_TELEMETRY_DISABLED=1 before every test, so the constructor returns at the environment-variable guard before it reaches disable_nonessential_telemetry. The assertion would still pass if the opt-out branch were broken or removed. Run this case without the global hard-disable (or inject the environment/1DS backend) and verify that nonessential records are suppressed while ProcessInfo remains allowed.

Stop collecting and emitting locale data from ProcessInfo telemetry while preserving the remaining startup metadata.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 297bc539-5f7f-4a70-9411-48e91a5bf532
@bmehta001 bmehta001 changed the title Fix telemetry opt-out wiring and action sampling Implement SDK v2 telemetry with privacy controls Aug 6, 2026
@bmehta001
bmehta001 requested a balanced review from Copilot August 6, 2026 06:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.

Suppressed comments (6)

sdk_v2/cpp/src/telemetry/telemetry_action_tracker.h:36

  • This correlation mechanism is currently unused in production: every ActionTracker caller passes the default context, no caller uses Context().AsIndirect(), and Session::ProcessRequest has no context parameter. A web route and the session work it triggers therefore receive unrelated correlation IDs and both report Direct=true, so the events cannot be joined as documented. Thread the parent context through the session/inference path.
  /// This action's context, with its correlation id resolved. Use to derive a
  /// child context (Context().AsIndirect()) for any caused-by action so all
  /// events from one operation share a correlation id.
  const InvocationContext& Context() const { return context_; }

sdk_v2/cpp/src/telemetry/telemetry.h:66

  • Normal HTTP 4xx paths never assign this status. Handlers construct an ActionTracker and then directly return ErrorResponse(400/404) for invalid input (for example, audio_transcriptions_handler.cc:93-100 and models_handlers.cc:54-62), leaving the default kFailure. This records client mistakes as service faults; set kClientError on every 4xx path or centralize status assignment in the response layer.
  kClientError,    // Rejected due to invalid client input (maps to HTTP 4xx) — not a service fault

sdk_v2/cpp/src/manager.cc:316

  • The 1DS sink is installed, but the new producers are not wired: repository-wide, RecordModelUsage, RecordAudioUsage, both EP methods, RecordDownload, RecordCatalogFetch, and RecordHardwareInfo have no production callers, and neither do StartSession/EndSession. As a result, the advertised end-to-end usage/download/catalog/EP/hardware/session telemetry is never emitted; only existing action/model-ID calls and this ProcessInfo call reach the sink. Please instrument the corresponding operations and Manager lifecycle before treating this as end to end.
  telemetry_ = std::make_unique<OneDsTelemetry>(config_.app_name, *logger_, disable_nonessential_telemetry);
  try {
    telemetry_->RecordProcessInfo(
        BuildProcessInfo(BuildTelemetryMetadata(config_.app_name),
                         !disable_nonessential_telemetry && !telemetry_hard_disabled));

sdk_v2/cpp/include/foundry_local/foundry_local_cpp.inline.h:186

  • This typed privacy setting is stored in the same mutable passthrough map as SetAdditionalOptions, so call order defeats authoritative opt-out precedence. For example, SetAdditionalOptions(Disable...=true) followed by this setter with false re-enables telemetry, unlike the internal typed || additional logic and the other SDK bindings. Add a dedicated C API configuration setter/field so the two opt-out sources remain independent and any true wins.
inline Configuration& Configuration::SetDisableNonessentialTelemetry(bool disable) {
  KeyValuePairs options;
  options.Set("DisableNonessentialTelemetry", disable ? "true" : "false");
  return SetAdditionalOptions(options);

sdk_v2/cpp/test/internal_api/telemetry_test.cc:152

  • This test cannot exercise the configuration opt-out: test_main.cc sets ORT_TELEMETRY_DISABLED=1 for every test, so the constructor returns on the hard-disable branch before reaching the disable_nonessential_telemetry branch. The assertion also passes when the argument is false, leaving the claimed opt-out/cache-isolation behavior untested. Use an injected/fake 1DS initializer or a no-network test hook to verify the configuration gate and selected cache separately.
    sdk_v2/cpp/src/telemetry/telemetry_redaction.h:45
  • Relative Windows paths with only one separator are not redacted. For example, failed at models\\alice.onnx reaches this branch with only one backslash, so it falls through and uploads the path, whereas the equivalent POSIX path is scrubbed. Detect a single trailing segment containing a dot here, matching the forward-slash logic, and add this case to the redaction test.
      size_t separators = 0;
      for (size_t j = i; j < s.size() && s[j] != '\r' && s[j] != '\n'; ++j) {
        if (s[j] == '\\' && ++separators >= 2) {
          return start;
        }
      }

Include resolved model IDs directly on Action events and remove the redundant standalone ModelId event so sampled action telemetry remains self-contained.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 297bc539-5f7f-4a70-9411-48e91a5bf532

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.

Suppressed comments (6)

sdk_v2/cpp/src/telemetry/telemetry_action_tracker.h:36

  • The new correlation context is never propagated: Context() has no production callers, service handlers create a default tracker, and Session::ProcessRequest creates another default tracker. Consequently, one HTTP operation's route action and downstream session/error/usage events receive unrelated correlation IDs, contrary to the documented grouping contract. Thread tracker.Context().AsIndirect() through the downstream session/inference calls.
  /// This action's context, with its correlation id resolved. Use to derive a
  /// child context (Context().AsIndirect()) for any caused-by action so all
  /// events from one operation share a correlation id.
  const InvocationContext& Context() const { return context_; }

sdk_v2/cpp/src/telemetry/telemetry.h:236

  • The typed telemetry surface is not wired to production producers. Repository-wide usage shows RecordModelUsage, RecordAudioUsage, RecordEpDownloadAttempt, RecordEpDownloadAndRegister, RecordDownload, RecordCatalogFetch, and RecordHardwareInfo are only called by telemetry implementations/tests, so normal inference, EP download, model download, catalog fetch, and startup flows never emit the events this PR advertises. Please inject/pass ITelemetry into those subsystems and populate these payloads at the operation boundaries.
  /// Record model usage metrics after inference (Model event).
  virtual void RecordModelUsage(const ModelUsageInfo& info) = 0;

  /// Record audio-specific inference metrics after audio inference (AudioModel event).
  virtual void RecordAudioUsage(const AudioUsageInfo& /*info*/) {}

sdk_v2/cpp/test/CMakeLists.txt:132

  • The custom main hard-disables telemetry only for foundry_local_tests and sdk_integration_tests. cache_only_tests still links GTest::gtest_main and its tests construct real Managers, so local runs outside CI can initialize 1DS and upload test telemetry. Apply the same test main/environment suppression to every C++ test executable, including cache_only_tests.
    sdk_v2/cpp/test/internal_api/telemetry_test.cc:148
  • This test cannot verify configuration opt-out: test_main.cc sets ORT_TELEMETRY_DISABLED=1 before every test, so OneDsTelemetry returns on the hard-disable branch before it evaluates disable_nonessential_telemetry. The assertion passes even if the configuration flag is completely ignored. Exercise this in an isolated process without the hard-disable variable, using a non-uploading fake/test hook, and assert that ProcessInfo remains allowed while nonessential events are blocked.
    sdk_v2/cpp/src/telemetry/telemetry.h:263
  • These lifecycle methods have no callers anywhere in the repository, so LogSession(Started/Ended) is never reached and 1DS session IDs/durations are not emitted. Start the telemetry session after manager initialization and end it before telemetry_ is reset during manager teardown.
  virtual void StartSession() {}
  virtual void EndSession() {}

sdk_v2/cpp/src/telemetry/telemetry.h:69

  • The detailed client-error status is not applied to HTTP validation failures. Handlers return ErrorResponse directly for empty bodies, invalid JSON, missing models, etc.; their ActionTracker therefore retains its default kFailure, and kClientError is only produced for thrown Foundry exceptions. Set the tracker status before 4xx returns (or centralize status derivation in the response path) so these events are not reported as service failures.
  kClientError,    // Rejected due to invalid client input (maps to HTTP 4xx) — not a service fault
  kCanceled,
  kDependencyFailure,
  kTimeout,

Suppress telemetry in cache-only tests, align usage-session boundaries with web-service lifetime, and classify HTTP 4xx validation responses as client errors.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 297bc539-5f7f-4a70-9411-48e91a5bf532

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 51 out of 51 changed files in this pull request and generated no new comments.

Suppressed comments (6)

sdk_v2/js/native/src/manager.cc:1

  • The condition has_disable_nonessential_telemetry && disable_nonessential_telemetry guarantees the value is true, so the ternary on line 217 is redundant and always sets "true". In addition, the duplicated else-if branch can be folded into the same path. A simpler approach is to set the KVP once when has_disable_nonessential_telemetry is true (setting either "true" or "false"), or if you intentionally only want opt-out, then remove the ternary and keep a single code path that only sets the key when the value is true.
    sdk_v2/cpp/test/internal_api/telemetry_test.cc:1
  • ScopedEnvVar uses TelemetryEnvironment::GetEnv() and treats original_.empty() as "was not set". This can’t distinguish between an unset env var and a var that was set to an empty string; in the latter case the destructor will incorrectly unset it. If these tests ever rely on restoring empty values, consider tracking presence separately (e.g., on POSIX using getenv(name) != nullptr, and on Windows checking GetEnvironmentVariableA + GetLastError()==ERROR_ENVVAR_NOT_FOUND) and storing the original value in an std::optional<std::string>.
    sdk_v2/cpp/src/telemetry/invocation_context.cc:36
  • MutableDefaultUserAgent() returns a process-global std::string that is read and written without synchronization. If SetDefaultUserAgent() can be called while other threads call DefaultUserAgent() (or InvocationContext::Direct()), this is a data race/UB. Consider protecting the global with a mutex, or switching to an atomic immutable representation (e.g., std::atomic<std::shared_ptr<const std::string>>) so reads are race-free; alternatively, document and enforce that SetDefaultUserAgent is only called during single-threaded startup.
std::string& MutableDefaultUserAgent() {
  static std::string user_agent = std::string("foundry-local-core/") + FOUNDRY_LOCAL_VERSION;
  return user_agent;
}

sdk_v2/cpp/src/telemetry/invocation_context.cc:49

  • MutableDefaultUserAgent() returns a process-global std::string that is read and written without synchronization. If SetDefaultUserAgent() can be called while other threads call DefaultUserAgent() (or InvocationContext::Direct()), this is a data race/UB. Consider protecting the global with a mutex, or switching to an atomic immutable representation (e.g., std::atomic<std::shared_ptr<const std::string>>) so reads are race-free; alternatively, document and enforce that SetDefaultUserAgent is only called during single-threaded startup.
std::string DefaultUserAgent() {
  return MutableDefaultUserAgent();
}

void SetDefaultUserAgent(std::string user_agent) {
  if (user_agent.empty()) {
    user_agent = std::string("foundry-local-core/") + FOUNDRY_LOCAL_VERSION;
  }
  MutableDefaultUserAgent() = std::move(user_agent);
}

sdk_v2/cpp/src/telemetry/device_id.cc:358

  • EnsureCacheDirectory() creates directories without symlink checks and then applies chmod() on the resolved path. On POSIX this can be abused if an attacker can replace the path with a symlink between calls, or if intermediate components are unexpectedly symlinks. Since this directory is used for telemetry/device identity state, it should be hardened similarly to the storage directory helpers (no symlink following; owner-only permissions). Consider reusing the existing owner-only directory creation logic (or adding a cache-specific hardened variant) and validating the leaf directory is not a symlink before creating/writing cache files.
std::filesystem::path TelemetryDeviceId::EnsureCacheDirectory() {
  auto dir = GetCacheDirectory();
  if (dir.empty()) {
    return {};
  }

  std::error_code ec;
  std::filesystem::create_directories(dir, ec);
  if (ec) {
    return {};
  }
#ifndef _WIN32
  ::chmod(dir.string().c_str(), S_IRWXU);
#endif
  return dir;
}

sdk_v2/cpp/src/manager.cc:323

  • BuildTelemetryMetadata() is called in OneDsTelemetry’s constructor and then called again here to build ProcessInfo. Since BuildTelemetryMetadata generates a new GUID and can call OS APIs (host app version, uname/RtlGetVersion, etc.), this duplicates work and may lead to subtle drift between the metadata used for common context and the values embedded in ProcessInfo. Consider building TelemetryMetadata once and reusing it (e.g., have OneDsTelemetry emit ProcessInfo as part of its own initialization, or accept precomputed metadata / a ProcessInfo payload), so startup work and identifiers remain consistent.
  const bool disable_nonessential_telemetry =
      config_.disable_nonessential_telemetry ||
      IsAdditionalOptionEnabled(config_, "DisableNonessentialTelemetry");
  const bool telemetry_hard_disabled =
      TelemetryEnvironment::IsCiEnvironment() || TelemetryEnvironment::IsTelemetryDisabledByEnvVar();
  telemetry_ = std::make_unique<OneDsTelemetry>(config_.app_name, *logger_, disable_nonessential_telemetry);
  try {
    telemetry_->RecordProcessInfo(
        BuildProcessInfo(BuildTelemetryMetadata(config_.app_name),
                         !disable_nonessential_telemetry && !telemetry_hard_disabled));
  } catch (const std::exception& ex) {
    logger_->Log(
        LogLevel::Warning,
        fmt::format("telemetry ProcessInfo failed during Manager initialization: {}", ex.what()));
  } catch (...) {
    logger_->Log(LogLevel::Warning, "telemetry ProcessInfo failed during Manager initialization.");
  }

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants