Integrate stable telemetry into MXC runtime - #820
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Promotes telemetry into MXC’s stable runtime configuration and integrates consent-aware emission across executors and the engine.
Changes:
- Adds stable telemetry parsing, authorization gates, and state-aware correlation.
- Introduces typed consent-maintenance requests and generated schemas.
- Adds Windows consent/ETW smoke tests and updates documentation.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/Build.Windows.Job.yml |
Adds telemetry tests and smoke coverage. |
docs/schema.md |
Documents stable telemetry configuration. |
docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md |
Updates telemetry correlation guidance. |
docs/telemetry/telemetry-consent-design.md |
Marks consent design as implemented. |
docs/telemetry/telemetry-consent-privacy-review.md |
Updates implementation status. |
docs/telemetry/telemetry-policy.md |
Marks policy integration as implemented. |
docs/telemetry/telemetry.md |
Updates telemetry implementation status. |
schemas/dev/mxc-config.schema.0.8.0-dev.json |
Promotes telemetry to the schema root. |
schemas/dev/mxc-telemetry-consent.schema.1.json |
Adds the maintenance-request schema. |
src/core/lxc/src/main.rs |
Adds consent maintenance and stable telemetry wiring. |
src/core/mxc_darwin/src/main.rs |
Adds equivalent macOS maintenance handling. |
src/core/mxc_engine/src/lib.rs |
Adds telemetry-aware streaming wrappers. |
src/core/mxc_engine/src/policy.rs |
Adds per-request telemetry enablement. |
src/core/mxc_engine/src/state_aware.rs |
Adds state-aware telemetry and correlation. |
src/core/wxc/src/main.rs |
Integrates maintenance commands and runtime emission. |
src/core/wxc_common/Cargo.toml |
Clarifies telemetry test-support behavior. |
src/core/wxc_common/src/config_parser.rs |
Parses stable telemetry and migration cases. |
src/core/wxc_common/src/models.rs |
Moves telemetry into ExecutionRequest. |
src/core/wxc_common/src/telemetry/consent_cli.rs |
Implements shared consent maintenance handling. |
src/core/wxc_common/src/telemetry/events.rs |
Updates ETW event documentation. |
src/core/wxc_common/src/telemetry/mod.rs |
Adds live authorization and SDK emission paths. |
src/core/wxc_common/src/wire.rs |
Defines telemetry maintenance wire contracts. |
src/testing/wxc_e2e_tests/tests/e2e_windows.rs |
Updates telemetry E2E invocation. |
src/tools/mxc_schema_gen/src/main.rs |
Adds consent schema/type generation modes. |
tests/examples/28_telemetry_enabled.json |
Uses stable top-level telemetry. |
tests/scripts/run_telemetry_consent_smoke_test.ps1 |
Adds isolated consent smoke coverage. |
tests/scripts/run_telemetry_etw_smoke_test.ps1 |
Strengthens isolated ETW smoke coverage. |
Suppressed comments (1)
src/core/wxc_common/src/telemetry/events.rs:106
- The provider emits
MXC.Error, notError; preserving this exact identity is part of the ETW smoke assertion. Keep the public function documentation aligned with the actual event contract.
/// Log an Error ETW event.
💡 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 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/telemetry/telemetry-consent-design.md:2
- Removing the implementation-status notice makes this document claim that the Rust/C ABI and Node/.NET consent APIs described later are available now, but this PR's own stack assigns those surfaces to follow-up PRs #821 and #822. Keep a partial-implementation notice until those PRs land so readers are not directed to APIs absent from this branch.
# Telemetry consent design
src/core/mxc_engine/src/lib.rs:195
- A timed-out state-aware streaming exec is converted to a generic
BackendError, soclassify_mxc_errorreportsprocess_errorinstead of the existingtimeouttelemetry category. PreserveErrorKind::TimedOutwhen constructing the state-aware telemetry outcome (or add a state-aware emit path that acceptsFailureReason::Timeout) so one-shot and state-aware timeout events agree.
let outcome = match result {
Ok(exit_code) => Ok(
wxc_common::state_aware_dispatch::DispatchOutcome::ExecCompleted {
exit_code: *exit_code,
},
),
Err(error) => Err(wxc_common::mxc_error::MxcError::backend_error(
error.to_string(),
)),
src/core/mxc_engine/src/policy.rs:787
SandboxRequestis re-exported bymxc-sdk, so this method immediately adds a public Rust SDK API. The repository's SDK-change convention requires updating the Rust SDK crate docs andsrc/core/mxc-sdk/README.mdin the same PR; neither currently documents this method. Please add those updates, or keep the method non-public until the SDK-exposure PR.
/// Enable or disable telemetry for this invocation.
///
/// Enabling this per-request switch is necessary but not sufficient:
/// telemetry still requires persisted user consent and an administrative
/// policy that permits collection. It is independent of experimental mode.
pub fn set_telemetry_enabled(&mut self, enabled: bool) -> &mut Self {
d297b29 to
b1e1b8b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 38 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
src/core/mxc_engine/src/state_aware.rs:86
- WSLc has been dropped from the state-aware dispatcher here.
resolve_backendstill returnsWslc, andwslc_common::WslcStateAwareRunnerstill implements the backend, but this request now falls through towxc_common::state_aware_dispatch::run_state_aware, which unconditionally returnsunsupported_phase. Restore the feature-gated WSLc arm and its experimental opt-in behavior.
if matches!(
backend,
wxc_common::models::ContainmentBackend::WindowsSandbox
| wxc_common::models::ContainmentBackend::IsolationSession
) && !parsed.request.experimental_enabled
src/core/mxc_engine/src/lib.rs:351
- A
try_waiterror does not establish that the sandbox has terminated; implementations can return a transient OS/transport error while the workload remains live. Emitting here releases the provider and suppresses the later real completion fromwait, producing an inaccurate terminal event. Leave telemetry active onErrso a later terminal operation (orDrop) accounts for the invocation.
// Poll-error branch — emit a synthesised terminal event so the
// provider reference isn't released without accounting for the
// run, then return the error unchanged to the caller.
Err(_) => self.emit_synthetic(SyntheticTerminal::PollError),
src/core/mxc_engine/src/lib.rs:367
- This records a successful cancellation even when the underlying
kill()returns an error and the process may still be running. That consumes the exactly-once telemetry slot, so a subsequent successfulwait()cannot report the actual outcome. EmitKilledonly after a successful kill.
let result = self.inner.kill();
self.emit_synthetic(SyntheticTerminal::Killed);
result
src/core/mxc_engine/src/lib.rs:250
- State-aware streaming timeouts are converted to
MxcError::backend_errorhere, soclassify_mxc_erroremitsprocess_errorinstead of the documentedtimeout.SandboxProcess::waitexplicitly reports timeouts asErrorKind::TimedOut; preserve that distinction when constructing the state-aware telemetry outcome.
let outcome = match result {
Ok(exit_code) => Ok(
wxc_common::state_aware_dispatch::DispatchOutcome::ExecCompleted {
exit_code: *exit_code,
},
),
Err(error) => Err(wxc_common::mxc_error::MxcError::backend_error(
error.to_string(),
)),
src/core/mxc_engine/src/lib.rs:280
Killedis encoded as a genericInterruptedI/O error, butemitonly special-casesTimedOut: one-shot mode therefore classifies this asinit_error, and state-aware mode asprocess_error. The public event contract includescancelled, so successful caller cancellation is currently misreported. Route this terminal kind through a cancellation-specific emission path.
SyntheticTerminal::Killed => (
std::io::ErrorKind::Interrupted,
"sandbox handle was killed before completion",
),
src/core/mxc_engine/src/state_aware.rs:123
- The WSLc streaming path has lost both its experimental gate and backend-specific dispatch. A
wslc:exec without opt-in now reports genericunsupported_phaseinstead ofbackend_unavailable, and an opted-in request no longer reachesWslcStateAwareRunner's deliberate library-consumer validation. Restore the feature-gated WSLc arm so the established error contract remains intact.
if matches!(
backend,
wxc_common::models::ContainmentBackend::WindowsSandbox
| wxc_common::models::ContainmentBackend::IsolationSession
) && !parsed.request.experimental_enabled
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/core/wxc_common/src/telemetry/events.rs:120
- The updated public event inventory removes
mxc.sandbox_kindfromMXC.Error, but this changed call still passes a value andmxc_telemetry::log_erroremits that field (src/mxc_telemetry/src/lib.rs:263). Align the provider payload and documentation before treating this as a stable contract.
sdk/node/src/types.ts:286 - This public SDK documentation still describes
enabled: trueas “force on,” although the promoted stable behavior also requires persisted user consent and permissive administrative policy. Update the interface docs so consumers do not expect this option to bypass those gates.
/** Telemetry configuration for TraceLogging ETW support. */
export interface TelemetryConfig {
/**
* Explicit telemetry override. `true` = force on, `false` = force off,
tests/scripts/run_telemetry_etw_smoke_test.ps1:221
- This CI smoke test now validates only that an event with the expected identity exists; it deleted the prior checks for
mxc.backend,mxc.exit_code,mxc.outcome, andmxc.duration_ms. The new Rust field-level ETW test is#[ignore]and is not invoked by this workflow, so payload regressions can pass CI. Retain field-level validation in the non-ignored smoke path.
Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Track a durable consent-record revision so same-state concurrent writes cannot be overwritten by a stale presenter response. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
a7bcb0f to
80085a4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/core/mxc_engine/src/lib.rs:366
- This emits
cancelledeven wheninner.kill()returns an error and the child remains running. It also consumes the terminal slot, so a subsequent successfulwait()cannot report the real outcome. Only finalize cancellation after termination is known to have succeeded; otherwise leave telemetry pending.
let result = self.inner.kill();
self.emit_synthetic(SyntheticTerminal::Killed);
src/core/wxc_common/src/wire.rs:105
- Promoting this field to the stable top level also requires updating the public schema reference.
docs/schema.md:127-129still presentsexperimental.telemetry, but the parser now rejects that legacy location, so users copying the documented example receive a migration error. Move the example and field documentation to the top-leveltelemetrysection in this PR.
src/core/mxc_engine/src/lib.rs:351 - A
try_waiterror does not mean the child has terminated, so consuming the one-shot telemetry slot here can record a terminalinternal_errorwhile the process continues. If a laterwait()succeeds, its real exit outcome is then suppressed. Keep the slot live on polling errors so a later terminal operation can report the actual outcome.
// Poll-error branch — emit a synthesised terminal event so the
// provider reference isn't released without accounting for the
// run, then return the error unchanged to the caller.
Err(_) => self.emit_synthetic(SyntheticTerminal::PollError),
src/core/mxc_engine/src/policy.rs:794
- Enabling telemetry after
build_request*creates a freshTelemetryConfigwithout the caller-requested containment kind. Consequently Rust SDK requests built with abstractContainment::Processreport the resolved backend (processcontainer,bubblewrap, orseatbelt) asmxc.sandbox_kind, contrary to the documented caller-intent field. Preserve the builder's requested containment when constructing this config.
pub fn set_telemetry_enabled(&mut self, enabled: bool) -> &mut Self {
self.inner.telemetry = Some(TelemetryConfig {
enabled: Some(enabled),
..Default::default()
});
docs/telemetry/telemetry.md:96
- The updated event contract removes
mxc.sandbox_kindfromMXC.Error, butmxc_telemetry::log_errorstill emits that field andevent_names_preserve_collector_contractexplicitly requires it. Align the provider/test and documentation so consumers have one authoritative payload contract.
| `mxc.backend` | string | Containment backend name |
Emit privacy product 11 and Client Diagnostic Data category 1 on every MXC event while retaining the approved Product and Service Usage tag. Document the optional non-CORE classification and lock the fields with contract tests. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
80085a4 to
9106ff8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/telemetry/telemetry.md:123
- The table now omits
mxc.sandbox_kind, butMXC.Errorstill emits that field:events.rs:116-122passes a sandbox kind tomxc_telemetry::log_error, and the provider contract test inmxc_telemetry/src/lib.rs:350-376requires it. Keep the public event documentation aligned with the actual payload, or remove the field from the provider and its contract test.
| Field | Type | Description |
|-------|------|-------------|
| `mxc.backend` | string | Containment backend name |
| `mxc.error_type` | string | Error category (`config_error`, `policy_error`, `process_error`, `timeout`, `init_error`, `internal_error`, `cancelled`, `unknown`) |
| `mxc.exit_code` | int32 | Process exit code |
| `mxc.phase` | string | State-aware lifecycle phase; empty for one-shot executions |
src/core/mxc_engine/src/lib.rs:252
- State-aware streaming timeouts are misclassified here.
ExecSandboxProcessexplicitly returnsio::ErrorKind::TimedOut(wxc_common/src/exec_stream.rs:545-551), but this branch converts every error toMxcError::backend_error, soemit_sdk_state_aware_with_kindrecordsprocess_errorinstead of the documentedtimeout. Preserve the timeout category when constructing the state-aware telemetry outcome.
Err(error) => Err(wxc_common::mxc_error::MxcError::backend_error(
error.to_string(),
)),
src/core/wxc/src/main.rs:1341
- This completion path drops the caller-requested containment kind captured in
request.telemetry.requested_sandbox_kind. Because parsing resolves abstractprocess/vminto a concreteContainmentBackendfirst (models.rs:89-119),emit_completionfalls back to that backend and emits the same value formxc.sandbox_kindandmxc.backend, contrary to the event contract indocs/telemetry/telemetry.md:104-108. Pass the retained requested kind through the executor emission path, as the SDK path already does.
telemetry::emit_completion(
telemetry_active,
&request.containment,
&response,
run_elapsed,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 54 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/core/mxc_engine/src/state_aware.rs:340
- The exec envelope cannot carry
containment, so the parser records the missing value as the one-shot defaultSome("process"). Using it here labels streaming exec telemetry for every state-aware VM/container backend asprocess, despite resolving the actual backend fromsandboxIdbelow. Non-provision state-aware requests should leave this attribution unset so emission uses the resolved backend.
let requested_sandbox_kind = parsed
.request
.telemetry
.as_ref()
.and_then(|config| config.requested_sandbox_kind);
sdk/node/src/types.ts:287
- This public description says
trueforces telemetry on, but the promoted stable contract only requests collection; persisted consent and administrative policy still gate emission. That wording can lead SDK users to believe this option bypasses consent. Match the generated wire documentation's opt-in semantics.
/** Telemetry configuration for TraceLogging ETW support. */
export interface TelemetryConfig {
/**
* Explicit telemetry override. `true` = force on, `false` = force off,
* `undefined` = off (default).
src/core/wxc_common/src/telemetry/events.rs:120
- This reconstructs
mxc.sandbox_kindfrom the resolved backend, so everyMXC.Errorloses the caller's abstract request (processorvm) even when its pairedMXC.Executionpreserves it via*_with_kind. The two events then disagree about the same invocation. Carry the request-scoped sandbox kind into the error emission instead of forcing the backend fallback.
src/core/mxc_engine/src/policy.rs:794 set_telemetry_enabledcreates telemetry after the wire config has already been parsed, sorequested_sandbox_kindremainsNone. For requests built with abstractContainment::ProcessorContainment::Vm, telemetry therefore reports the resolved concrete backend rather than the caller-requested kind documented formxc.sandbox_kind. Preserve the original containment intent when buildingSandboxRequestand copy it into this config.
pub fn set_telemetry_enabled(&mut self, enabled: bool) -> &mut Self {
self.inner.telemetry = Some(TelemetryConfig {
enabled: Some(enabled),
..Default::default()
});
src/core/mxc_engine/src/state_aware.rs:252
- For every non-provision phase,
containmentis intentionally absent, and the shared parser consequently storesrequested_sandbox_kind = Some("process"). Passing that value here misattributes start/stop/deprovision telemetry for Windows Sandbox, IsolationSession, and WSLC asprocessinstead of falling back to the backend resolved fromsandboxId. Treat the requested kind as absent on non-provision envelopes.
This issue also appears on line 336 of the same file.
let requested_sandbox_kind = parsed
.request
.telemetry
.as_ref()
.and_then(|config| config.requested_sandbox_kind);
docs/telemetry/telemetry.md:120
- The
MXC.Errorprovider still emitsmxc.sandbox_kind(src/mxc_telemetry/src/lib.rs:280), so removing it from this payload table makes the telemetry data inventory incomplete. Restore the field to keep the privacy-facing documentation aligned with the actual event contract.
| `mxc.backend` | string | Containment backend name |
Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Exercise the task's wake notification directly instead of depending on two nested threads completing within a two-second scheduler window. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Keep terminal telemetry active when the wrapped kill fails, and classify successful one-shot and state-aware kills as cancelled with the documented exit code. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Require live decoded MXC events to carry the mandatory privacy product and Client Diagnostic Data category fields alongside the approved privacy tag. Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
Co-authored-by: Copilot <[email protected]> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
a7385b3 to
253228a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 54 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/core/mxc_engine/src/lib.rs:386
- A
try_wait()error does not prove that the sandbox terminated. For example, the Windows implementations returnErrwhenWaitForSingleObjectorGetExitCodeProcessfails while the process may still be running. Consumingactivehere releases the provider and prevents a later successfulwait()from reporting the real outcome, which is the same lifecycle problem already avoided for a failedkill(). Keep the telemetry slot active after polling errors unless termination is known.
// Poll-error branch — emit a synthesised terminal event so the
// provider reference isn't released without accounting for the
// run, then return the error unchanged to the caller.
Err(_) => self.emit_synthetic(SyntheticTerminal::PollError),
src/core/mxc_engine/src/lib.rs:252
- This state-aware branch converts every wait error to
BackendError, includingErrorKind::TimedOut;classify_mxc_errorthen reportsprocess_errorinstead of the definedtimeoutcategory. The one-shot branch immediately above preservesFailurePhase::Timeout, so streaming state-aware executions currently produce inconsistent and inaccurate telemetry. Preserve the timeout distinction in the state-aware emission path as well.
let outcome = match result {
Ok(exit_code) => Ok(
wxc_common::state_aware_dispatch::DispatchOutcome::ExecCompleted {
exit_code: *exit_code,
},
),
Err(error) => Err(wxc_common::mxc_error::MxcError::backend_error(
error.to_string(),
)),
src/testing/wxc_e2e_tests/tests/e2e_telemetry_etw.rs:222
- The instrumented child redirects only the consent store, so it still reads the host's real administrative policy. On a managed machine with
AllowTelemetry=0,assert_effective_consentstill sees"granted", buttelemetry::initcorrectly suppresses emission and the granted trace assertion fails. Redirect policy to an isolated allowed HKCU test key (as the PowerShell smoke test does) for both status and execution commands.
.github/copilot-instructions.md.Summary
Promotes telemetry to the stable top-level configuration and integrates the reviewed consent and policy foundation with the engine and executors. Adds typed maintenance requests, live authorization checks, generated schemas, migration handling, and Windows consent/ETW smoke coverage.
This is PR 3 of 5 and depends on the consent/policy foundation in PR 2.
Stack
Review only this PR's diff; prerequisite behavior is in the PR above.