Repository navigation
Extend Required-only telemetry to all supported contracts - #1733
Richie Gomez (richiemsft) wants to merge 4 commits into
Conversation
Apply the approved four-event inventory to 0.9.0-alpha and 1.0.0 as well as 1.1.0-alpha. Preserve selected typed lifecycle attribution, unattributed-request behavior, consent management, and build gates. Extend existing functional CI scenarios and align SDK documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd8d86b8-5fcd-4f6d-a834-d126b5c3d4d9
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The telemetry change breaks published V1 behavior by emitting Required events despite disabled telemetry or withdrawn consent and dropping optional events, requiring explicit maintainer acceptance or preservation of V1 behavior before shipping.
3 open issues
What changed in this PR
Extends MXC’s Required-only Windows telemetry behavior to 0.9.0-alpha and 1.0.0, alongside 1.1.0-alpha, following the earlier telemetry and signed-routing changes.
Changes:
- Shares contract selection between telemetry initialization and event inventory, and preserves typed Rust lifecycle attribution.
- Expands ETW validation to three contracts, including disabled request telemetry, denied consent, and restrictive policy.
- Updates SDK comments and documentation to explain Required collection and the intentional V1 compatibility exception.
| File | Description |
|---|---|
| tests/scripts/test_telemetry_etw_assertions.ps1 | Expands offline trace controls and request validation to three contracts. |
| tests/scripts/run_telemetry_etw_smoke_test.ps1 | Exercises Required collection across contracts and consent/policy states. |
| tests/scripts/run_isolation_session_state_aware_tests.ps1 | Runs one telemetry lifecycle per supported contract. |
| tests/scripts/README.md | Updates telemetry test coverage and prerequisites. |
| tests/scripts/lib/TelemetryEtwAssertions.ps1 | Checks three lifecycles and Required event metadata. |
| src/mxc-sdk/tests/wxc_e2e_tests_e2e_telemetry_etw.rs | Updates live ETW tests for consent-independent Required collection. |
| src/mxc-sdk/src/telemetry.rs | Clarifies optional-consent API documentation. |
| src/mxc-sdk/src/state_aware_sdk.rs | Updates contract-attribution expectations in tests. |
| src/mxc-sdk/src/options.rs | Documents telemetry options as compatibility preferences. |
| src/mxc-sdk/src/core/mxc_engine/verbose_telemetry.rs | Tests verbose-transport exclusion for all supported contracts. |
| src/mxc-sdk/src/core/mxc_engine/state_aware.rs | Tests lifecycle telemetry with opt-out, denied consent, and restrictive policy. |
| src/mxc-sdk/src/core/mxc_common/telemetry/session.rs | Uses the shared Required-event selector for sessions. |
| src/mxc-sdk/src/core/mxc_common/telemetry/mod.rs | Adds the shared contract selector and expands authorization tests. |
| src/mxc-sdk/src/core/mxc_common/telemetry/events.rs | Updates supported-contract event inventory expectations. |
| src/mxc-sdk/src/core/mxc_common/sdk_input.rs | Documents retained typed lifecycle contract attribution. |
| src/mxc-sdk/src/core/mxc_common/models.rs | Clarifies telemetry preference and contract-attribution comments. |
| src/mxc-sdk/src/core/mxc_common/config_parser.rs | Preserves contract attribution during typed lifecycle normalization. |
| src/mxc-sdk/README.md | Explains Required telemetry behavior for Rust callers. |
| sdk/node/src/v1/types.ts | Updates telemetry configuration comments only. |
| sdk/node/README.md | Explains Required collection and consent-management limits. |
| sdk/dotnet/README.md | Explains Required telemetry behavior for .NET callers. |
| sdk/dotnet/Microsoft.Mxc.Sdk/V1/LifecycleTypes.cs | Updates lifecycle telemetry comments only. |
| sdk/dotnet/Microsoft.Mxc.Sdk/V1/ExecutionOptions.cs | Updates creation-option telemetry comments only. |
| sdk/dotnet/Microsoft.Mxc.Sdk/V1/ContainerRequestSections.cs | Clarifies telemetry configuration comments only. |
| README.md | Describes Required telemetry across supported contracts. |
| docs/telemetry.md | Updates collection, compatibility, and consent guidance. |
| docs/schema.md | Clarifies the telemetry field’s compatibility role. |
| docs/development/build-and-test/ci-validation-infrastructure.md | Documents expanded ETW validation scenarios. |
| docs/development/architecture/versioning.md | Records the intentional telemetry compatibility exception. |
| docs/development/architecture/telemetry.md | Updates event selection, authorization, and acceptance guidance. |
| docs/development/architecture/telemetry-consent-design.md | Separates consent management from Required collection. |
| docs/api-reference/rust/v1/types.md | Updates the Rust telemetry type description. |
| docs/api-reference/rust/v1/api.md | Clarifies Rust consent API semantics. |
| docs/api-reference/node/v1/types.md | Updates the Node telemetry type description. |
| docs/api-reference/node/v1/api.md | Clarifies Node consent API semantics. |
| docs/api-reference/dotnet/v1/types.md | Updates the .NET telemetry type description. |
| docs/api-reference/dotnet/v1/api.md | Clarifies .NET consent API semantics. |
| .github/copilot-instructions.md | Aligns contributor guidance with Required telemetry behavior. |
Review effort: Balanced
Keep the supported-contract matrix running on every platform, but expect Windows-only consent persistence to fail on Linux and macOS. Preserve Windows persistence checks and every build-eligibility assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd8d86b8-5fcd-4f6d-a834-d126b5c3d4d9
…identities Address Copilot review feedback by retaining the selected V1 contract in the creation adapter and testing Required events through public v1::run with opt-out and denied consent. Check session/root uniqueness across all three ETW lifecycles with consistent-reuse negative controls. Preserve genuinely unattributed requests and the approved compatibility decision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd8d86b8-5fcd-4f6d-a834-d126b5c3d4d9
There was a problem hiding this comment.
🔵 Needs a closer look
Expanded consent-free collection requires human review of live Windows ETW lifecycle, signed-component routing, and ingestion acceptance evidence that offline and local test-mode results do not establish.
No open issues 🎉
4 resolved since last review
Review effort: Balanced
Carlos Alexandro Becker (caarlos0)
left a comment
Carlos Alexandro Becker (caarlos0)
left a comment
There was a problem hiding this comment.
Tentative approval, pending resolution of inline feedback. Reviewed the complete change with independent Anthropic and OpenAI checks: one agreed, previously reported compatibility alert; one additional finding survived verification; two other concerns were excluded as pre-existing or outside the agreed scope.
The P0 / SDK breaking change alert in the existing thread remains a compatibility alert even though the change is intentional. This approval does not independently establish release-owner acceptance of the documented V1 exception; it does not add a duplicate inline comment.
All reported checks for 389741b17daf97ada8ce5d4523ba6ebb00324a31 passed. Windows x64/arm64 job evidence includes live ETW smoke coverage; the public Rust creation regression and lifecycle-3 reuse controls also ran. No local builds were run. Local tests were skipped because host load exceeded the review limit and this host cannot capture Windows ETW. Head-specific scheduled IsolationSession capture, the ignored standalone Rust ETW tests, and signed-component ingestion acceptance were not verified.
– Copilot, on behalf of Carlos
Keep containment attribution on the internal execution request independently of optional telemetry settings. Preserve the Rust Process alias, update engine and executor readers, and regress JSON and public V1 emission without changing preference presence or policy hashes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd8d86b8-5fcd-4f6d-a834-d126b5c3d4d9
There was a problem hiding this comment.
🔵 Needs a closer look
Human review is needed because Windows checks remain in progress and the available evidence does not establish live three-contract ETW lifecycle capture or signed-component ingestion acceptance for this revision.
No open issues 🎉
Review effort: Balanced
|
CI diagnosis for a4f8f19 / run 43094292584: the four failed jobs are infrastructure failures in artifact upload, not failed compiler/test steps. Linux x64 and ARM64 plus macOS ARM64 failed Upload binaries; Windows ARM64 failed Upload local-only telemetry test executor. The artifact API returned HTML (<!DOCTYPE ...) instead of JSON after five attempts (CreateArtifact / FinalizeArtifact). Build/test steps before those uploads succeeded. Windows ARM64 consent/ETW/release smoke steps after the failed upload were skipped, so those checks are not claimed passed. GitHub Status currently reports a major Actions outage (November 11, 2026), consistent with these failures. No code or CI-gate changes are warranted. Leave retries until service recovery, then rerun the failed jobs and dependent checks. |
|
Attempt 2 follow-up: Linux x64/ARM64 and macOS ARM64 recovered and passed. Windows ARM64 now failed in the unchanged isolation_session integration test dev_json_lifecycle_preserves_execution_and_caller_ownership at captured exec. The native IsoSessionOps.RunProcessWithOptionsAsync API reported failure to initialize its worker process wrapper: 0x8007042a, extendedError=0x8007001F. This same scenario passed on ARM64 in attempt 1 of the identical a4f8f19 commit, and on Windows x64. This suggests an intermittent native-host failure, not another artifact outage; it does not prove a root cause. Started a targeted retry of Windows ARM64 and dependent checks, without changing code, skipping the scenario, or adding retries inside the test. Results are pending. |
|
Attempt 3 completed successfully on unchanged commit a4f8f19. Windows ARM64 passed, including dev_json_lifecycle_preserves_execution_and_caller_ownership (the previously failing scenario), the isolated telemetry ETW smoke test, release-path consent test, and artifact uploads. The full Build workflow and all 31 currently reported PR checks are successful, including dependent SDK integration/package checks. No code changes, assertion changes, or test-local retries were needed. |
|
Gudge (@MGudgin) / Branden Bonaby (@bbonaby) can you take a look? |
📖 Description
Extend the existing Required-only telemetry path to
0.9.0-alphaand1.0.0, alongside1.1.0-alpha, on eligible Windows builds. All three contracts collect onlyMXC.ExecutionStart,MXC.ExecutionEnd,MXC.Error, andMXC.ProcessExited, regardless of request opt-in, stored consent, or administrative policy.SDK breaking change — P0 compatibility alert, intentional: existing V1 callers can no longer suppress these four events with
enabled: false, consent withdrawal, or administrative policy. The older contracts also stop emitting optional event families. This implements the already-approved design confirmed by the requester; the specific exception is documented in the versioning guide. No separate approved spec URL was supplied, so the spec-link checklist item remains unchecked. This is a behavioral compatibility alert, not a claim of a security vulnerability.One shared, explicit three-contract selector now controls both initialization and session event inventory. Typed Rust creation and lifecycle normalization preserve their already-selected contract so they follow the same Required path as JSON-based callers. Truly unattributed internal requests retain their existing request/consent/policy gates and optional inventory.
Requested containment attribution is independent of the optional telemetry preference. JSON
"process"and Rust V1Containment::Processretain sandbox kind"process"even when the resolved backend is ProcessContainer and telemetry is omitted. This metadata is internal, is not serialized, and does not change policy hashes or optional-field presence.Build eligibility, signed routing, debug test-mode restrictions, non-Windows no-ops, provider ownership, event identities/payloads, local diagnostics, and execution/cleanup behavior are unchanged. Consent-management API/CLI signatures, stored records, serialization, SDK contract targets, and released schemas are unchanged.
Existing Windows ETW smoke and scheduled IsolationSession scenarios now cover all three contracts. The trace oracle checks actual Required classification and privacy tags, rejects all excluded event families, and retains failure, process-exit, correlation, and cleanup assertions. SDK documentation and public comments explain the changed collection behavior.
🔗 References
Refs #1588; follows #1575 and #1673. The related issue is already closed; this PR does not change issue state.
See
docs/telemetry.md,docs/development/architecture/versioning.md#approved-telemetry-behavior-change, and the existing telemetry architecture/CI runbook.🔍 Validation
Rust commands were run from
src\. Required-eligible local checks used$env:MXC_TELEMETRY_COMPONENT_PATH=''and$env:MXC_TELEMETRY_TEST_KEYWORDS='1'; ordinary-debug checks left the component empty and removed the keyword setting. These local test builds have no Microsoft routing.cargo test --locked -p mxc-sdk --lib --features test-support,isolation_session -- telemetry mxc_engine exec_stream owned_thread state_aware_sdk config_parser sdk_inputcargo fmt --all -- --checkcargo clippy --locked --workspace --all-targets --features test-support,isolation_session -- -D warningscargo build --locked -p wxc --features test-support,isolation_sessiontests\scripts\test_telemetry_etw_assertions.ps1 -WxcExePath src\target\debug\wxc-exec.exetests\scripts\run_telemetry_consent_smoke_test.ps1 -BinDir src\target\debugnode scripts\check-telemetry-policy-parity.jsnpm testfromsdk\node\git diff --checktests\scripts\run_telemetry_etw_smoke_test.ps1 -BinDir src\target\debug -CheckKeywordsPowerShell parser checks passed for all four modified
.ps1files.Regression-before/fix-after evidence: temporarily restoring the original development-only selector made all three selected regression tests fail; restoring the explicit supported-contract selector passed:
An intermediate eligible run failed with Windows
Access is deniedin the unchanged consent-store testwrite_through_replace_handles_new_and_existing_records. The subsequent complete 532-test run passed; no consent-store implementation or assertion was changed.CI follow-up: the initial Linux ARM64 and macOS jobs exposed a test-setup error: the cross-platform telemetry matrix unconditionally unwrapped Windows-only consent persistence. The test now requires persistence to succeed on Windows and to return an error elsewhere; all contract/build-eligibility assertions still run on every platform. No production behavior or CI gate was changed. Initial Linux x64 separately failed in the unchanged Bubblewrap descendant-probe test with
Text file busy (os error 26); that code is outside this PR.Follow-up local validation passed:
src\cargo test --locked -p mxc-sdk --lib --features test-support,isolation_session -- telemetrycargo check --locked -p mxc-sdk --lib --tests --target x86_64-unknown-linux-gnucargo fmt --all -- --checkcargo clippy --locked -p mxc-sdk --lib --tests --features test-support,isolation_session -- -D warningsOn follow-up commit
c60c99d7, CI run43082310397passed the previously failing macOSTeststep and both Linux architectures'Test common contracts and state-aware SDK boundariessteps. Linux x64 also passed the unchanged Bubblewrap version-module tests that previously encounteredText file busy. Remaining job steps and other CI checks are still running; this is not a claim that the entire workflow has passed.Copilot review follow-up: preserve
1.0.0attribution in the Rust creation adapter, not just lifecycle normalization. A regression calls publicv1::runwith telemetry omitted/false, denied consent, and policy0; it checks matching Required start/error/end records for the unsupported-backend failure and no optional events. Existing Windows release workspace tests run this public-API scenario. The ETW oracle now requires unique session IDs and correlation roots across all three lifecycles; four new negative controls consistently reuse lifecycle 1/2 attribution throughout lifecycle 3.Both regressions failed before their respective fixes: the public API emitted zero starts rather than one in the Required-eligible build, and the old oracle accepted the consistently reused third lifecycle. After fixing them:
cargo test --locked -p mxc-sdk --lib --features test-support,isolation_session -- telemetry policy::tests policy::exact policy::sdk_v1_conformance configs::process_container v1::tests mxc_engine::run::attribution_testsfromsrc\cargo fmt --all -- --checkfromsrc\cargo clippy --locked --workspace --all-targets --features test-support,isolation_session -- -D warningsfromsrc\cargo check --locked -p mxc-sdk --lib --tests --target x86_64-unknown-linux-gnufromsrc\tests\scripts\test_telemetry_etw_assertions.ps1 -WxcExePath src\target\debug\wxc-exec.exeAttribution review follow-up: move requested-containment metadata from optional telemetry settings to the internal execution request, and update the shared engine and executor readers. The typed creation adapter retains the original containment before backend resolution. No public SDK signatures or schemas change.
The new JSON/public Rust regression reproduced the reported failure before the fix: the omitted-telemetry JSON request emitted sandbox kind
"processcontainer"instead of"process". After the fix, actual JSON and publicv1::runinvocations with omitted/false telemetry assert"process"on both start and end records under denied consent and policy0. These tests run in the existing Windows release workspace suite. Parser coverage additionally checks all three contracts with omitted telemetry, an empty object, and explicit false while preserving optional-field presence. Existing policy-hash and lifecycle-binding coverage passes.cargo test --locked -p mxc-sdk --lib --features test-support,isolation_session -- telemetry policy::tests policy::exact policy::sdk_v1_conformance configs::process_container v1::tests mxc_engine state_aware_binding policy_identity config_parserfromsrc\cargo fmt --all -- --checkfromsrc\cargo clippy --locked --workspace --all-targets --features test-support,isolation_session -- -D warningsfromsrc\cargo check --locked -p mxc-sdk -p lxc --all-targets --target x86_64-unknown-linux-gnufromsrc\cargo build --locked -p wxc --features "test-support isolation_session"fromsrc\tests\scripts\test_telemetry_etw_assertions.ps1 -WxcExePath src\target\debug\wxc-exec.exegit diff --checkAll reported checks on
389741b1passed before this attribution follow-up; checks for the new follow-up commit are pending.Not run locally: live IsolationSession ETW capture (
tests\scripts\run_telemetry_etw_smoke_test.ps1 -BinDir src\target\debug -IsolationSessionLifecycle -CheckKeywords -SkipClean), privileged release real-store/HKLM mutation tests, and signed-component ingestion acceptance. Existing Windows build jobs and scheduledisolation-sessionvalidation retain their wiring for these scenarios. Local oracle/unit results do not prove Microsoft ingestion. .NET runtime/AOT tests were not run because .NET edits are comments only. CI results for the latest changes are pending.✅ Checklist
feature-acceptedCargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 Issue Type