Add Node.js and .NET telemetry SDKs - #822
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. |
| action is required to keep it disabled. | ||
|
|
||
| Omitting either (the default) turns telemetry off entirely. On non-Windows platforms all telemetry functions are no-ops. | ||
| Those settings are necessary but not sufficient: on Windows, explicit |
| - **MXC owns its own consent state.** It must never read or infer from the Windows system telemetry consent. The consent store is a per-user JSON file; the policy is `HKLM\SOFTWARE\Policies\Mxc` → `AllowTelemetry` (`REG_DWORD`). | ||
| - **One definition, distributed to the bindings.** The Rust `ConsentState` / `PolicyState` enums are the source of truth; the FFI, C#, and TypeScript layers marshal the same strings. `scripts/check-telemetry-policy-parity.js` fails if the four `PolicyState` spellings drift apart and runs in the versioning checks workflow. | ||
| - **Test isolation.** The consent store and the policy key are process-global, each behind its own mutex. Use `wxc_common::telemetry::test_support::TelemetryTestEnv` whenever a test needs both; constructing `PolicyKeyGuard` and `LocalAppDataGuard` directly in the same test risks a lock-order deadlock. Both overrides are `cfg(debug_assertions)`-gated, so the smoke test refuses to run against a release binary. The `wxc_common` `test-support` feature re-exports the policy override for downstream crates' integration tests (`mxc_ffi` uses it) and must stay a dev-dependency-only feature. | ||
| - **Read-only queries must never be able to crash the host.** `NeedsConsentPrompt`/`needsTelemetryConsentPrompt` and `GetPolicy`/`getTelemetryPolicy` fail closed on *any* failure and never throw — including a non-`Success` FFI status, which covers a caught panic. The consent *read* and *write* still throw, because their callers must distinguish "not decided" from "could not read" and "did not persist"; when they do, they raise only the binding's documented exception type (`MxcException`), wrapping anything unexpected rather than letting a raw type escape. |
There was a problem hiding this comment.
This might be overkill/paranoid? I'm concerned that the agent might drop in code where telemetry leads to an uncaught failure.
| - When the policy is `Blocked`, `NeedsConsentPrompt()` returns `false`, | ||
| because asking for permission an administrator has already refused is a | ||
| meaningless question. Word any UI as "telemetry is unavailable on this | ||
| device" rather than blaming the user's own choice. |
There was a problem hiding this comment.
Lots of copy-pasta from other places.
microsoft-github-policy-service
Bot
added
the
Copilot-Instructions
There was a problem hiding this comment.
Pull request overview
Adds stable telemetry consent, policy, and per-run controls to the Node.js and .NET SDKs over the native telemetry contract.
Changes:
- Adds presenter-driven consent and fail-closed policy APIs.
- Propagates telemetry through one-shot and state-aware execution.
- Adds tests, parity/codegen checks, and SDK documentation.
Reviewed changes
Copilot reviewed 33 out of 38 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/scripts/run_telemetry_consent_smoke_test.ps1 |
Normalizes the consent smoke test. |
src/core/wxc_common/src/telemetry/policy.rs |
Normalizes telemetry policy source. |
sdk/node/tests/unit/wire-conformance.test.ts |
Checks telemetry wire conformance. |
sdk/node/tests/unit/telemetry.test.ts |
Tests Node consent APIs. |
sdk/node/tests/unit/state-aware.test.ts |
Tests state-aware telemetry propagation. |
sdk/node/tests/unit/sandbox.test.ts |
Tests one-shot telemetry options. |
sdk/node/src/types.ts |
Promotes telemetry configuration. |
sdk/node/src/telemetry.ts |
Implements Node consent APIs. |
sdk/node/src/state-aware.ts |
Relays state-aware telemetry. |
sdk/node/src/state-aware-types.ts |
Updates telemetry documentation. |
sdk/node/src/state-aware-helper.ts |
Builds top-level telemetry envelopes. |
sdk/node/src/sandbox.ts |
Adds per-invocation telemetry options. |
sdk/node/src/index.ts |
Exports telemetry APIs. |
sdk/node/src/helper.ts |
Applies one-shot telemetry overrides. |
sdk/node/src/generated/wire.ts |
Updates generated stable wire types. |
sdk/node/src/generated/telemetry-consent-wire.ts |
Adds generated consent wire types. |
sdk/node/README.md |
Documents Node telemetry usage. |
sdk/node/package.json |
Runs telemetry unit tests. |
sdk/dotnet/README.md |
Documents .NET telemetry usage. |
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryPolicyState.cs |
Defines policy states. |
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryConsentState.cs |
Defines consent states. |
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryConsent.cs |
Defines consent models. |
sdk/dotnet/Microsoft.Mxc.Sdk/StateAwareTypes.cs |
Adds lifecycle telemetry options. |
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxPolicy.cs |
Adds one-shot telemetry opt-in. |
sdk/dotnet/Microsoft.Mxc.Sdk/Native/NativeLibraryResolver.cs |
Improves native profile resolution. |
sdk/dotnet/Microsoft.Mxc.Sdk/MxcTelemetry.cs |
Implements .NET consent APIs. |
sdk/dotnet/Microsoft.Mxc.Sdk/MxcLifecycle.cs |
Propagates lifecycle telemetry. |
sdk/dotnet/Microsoft.Mxc.Sdk/MxcException.cs |
Preserves underlying exceptions. |
sdk/dotnet/Microsoft.Mxc.Sdk/Microsoft.Mxc.Sdk.csproj |
Builds profile-specific native libraries. |
sdk/dotnet/Microsoft.Mxc.Sdk/ErrorCode.cs |
Adds consent-write failure code. |
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcTelemetryTests.cs |
Tests .NET consent and policy behavior. |
sdk/dotnet/Microsoft.Mxc.Sdk.Tests/MxcLifecycleTests.cs |
Tests lifecycle telemetry envelopes. |
scripts/versioning/check-telemetry-consent-codegen.js |
Verifies generated consent artifacts. |
scripts/check-telemetry-policy-parity.js |
Checks cross-language policy states. |
README.md |
Documents stable telemetry behavior. |
docs/telemetry/telemetry-policy.md |
Normalizes administrative policy docs. |
.github/workflows/Versioning.Checks.Job.yml |
Adds telemetry parity checks. |
.github/copilot-instructions.md |
Records telemetry architecture and commands. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| presenter: TelemetryConsentPresenter, | ||
| ) => Promise<TelemetryConsentMaintenanceResponse>; | ||
|
|
||
| const CONSENT_REQUEST_TIMEOUT_MS = 30_000; |
|
|
||
| /** | ||
| * Telemetry configuration for experimental TraceLogging ETW support. | ||
| * Telemetry configuration for TraceLogging ETW support. |
| `'allowed'` does not grant user consent, while `'blocked'` disables collection | ||
| and the consent prompt. An unreadable or missing `policy` field reads back as | ||
| `'blocked'`; non-Windows hosts return `'not-applicable'`. See | ||
| [`docs/telemetry/telemetry-administrative-policy.md`](https://github.com/microsoft/mxc/blob/main/docs/telemetry/telemetry-administrative-policy.md). |
| `Allowed` does not grant user consent, while `Blocked` disables collection and | ||
| the consent prompt. The policy query fails closed to `Blocked` if the native | ||
| library cannot be loaded; non-Windows hosts return `NotApplicable`. See | ||
| [`docs/telemetry/telemetry-administrative-policy.md`](../../docs/telemetry/telemetry-administrative-policy.md). |
|
|
||
| /// <summary> | ||
| /// The administrative (MDM / Group Policy) telemetry decision for this machine. | ||
| /// See docs/telemetry/telemetry-administrative-policy.md for the admin-facing reference. |
| const csharpStates = new Set(); | ||
| for (const m of parseBody[1].matchAll(/"([a-z-]+)"\s*=>/g)) { | ||
| csharpStates.add(m[1]); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 38 changed files in this pull request and generated no new comments.
Suppressed comments (6)
sdk/dotnet/README.md:133
- The referenced
telemetry-administrative-policy.mdfile does not exist; the policy guide added to the repository isdocs/telemetry/telemetry-policy.md. Point this link at the actual document.
`Allowed` does not grant user consent, while `Blocked` disables collection and
the consent prompt. The policy query fails closed to `Blocked` if the native
library cannot be loaded; non-Windows hosts return `NotApplicable`. See
[`docs/telemetry/telemetry-administrative-policy.md`](../../docs/telemetry/telemetry-administrative-policy.md).
sdk/node/README.md:516
- This link targets
telemetry-administrative-policy.md, which does not exist; the administrative policy document in this PR isdocs/telemetry/telemetry-policy.md. Update the URL so published package documentation does not lead to a 404.
`'allowed'` does not grant user consent, while `'blocked'` disables collection
and the consent prompt. An unreadable or missing `policy` field reads back as
`'blocked'`; non-Windows hosts return `'not-applicable'`. See
[`docs/telemetry/telemetry-administrative-policy.md`](https://github.com/microsoft/mxc/blob/main/docs/telemetry/telemetry-administrative-policy.md).
sdk/dotnet/Microsoft.Mxc.Sdk/TelemetryPolicyState.cs:8
- This XML documentation references a nonexistent
telemetry-administrative-policy.md; the repository's administrator-facing guide isdocs/telemetry/telemetry-policy.md. Correct the path so generated API docs direct users to a real file.
/// <summary>
/// The administrative (MDM / Group Policy) telemetry decision for this machine.
/// See docs/telemetry/telemetry-administrative-policy.md for the admin-facing reference.
sdk/node/src/telemetry.ts:153
- A presenter may legally
throw undefinedin JavaScript. In that case this assignment leavespresenterFailureequal to the “no failure” sentinel, so the close handler can resolve the dismissed native response instead of propagating the presenter failure. Track failure with a separate boolean or normalize every caught value to anError.
sdk/node/src/telemetry.ts:353 - This convenience getter discards
queryTelemetryConsent().error, so a missing/mismatched executable is indistinguishable from a genuine undecided user. Consent reads are required to surface failures through the binding's documented exception type; only prompt/policy queries should silently fail closed. Check the query result and throw anMxcErrorwhenerroris present.
sdk/dotnet/Microsoft.Mxc.Sdk/MxcTelemetry.cs:65 - Returning
Undeterminedfor native-load failures makes a broken or outdated native installation indistinguishable from a user who has not decided. Consent reads must raise the documentedMxcExceptionon infrastructure failures; reserve silent fail-closed behavior forNeedsConsentPromptandGetPolicy. Wrap this failure asMxcExceptioninstead of returning a consent state.
catch (Exception ex) when (IsNativeLoadFailure(ex))
{
ReportFailClosed("GetConsent", "Undetermined", ex);
return TelemetryConsentState.Undetermined;
}
| yield return Path.Combine(baseDir, "runtimes", RuntimeInformation.RuntimeIdentifier, "native", file); | ||
|
|
||
| // Dev layout: walk up looking for the Cargo target dir. | ||
| // Dev layout: walk up looking for the Cargo target dir. Probe the |
There was a problem hiding this comment.
High (security) — Do not let ancestor Cargo paths shadow the application's packaged native DLL.
This PR moves the walk-up src/target/<profile> probes ahead of baseDir and runtimes/<rid>/native. The resolver accepts the first existing DLL without validating repository ownership, signature, or version. An application under an ancestor where another user can plant src/target/release/mxc_ffi.dll can therefore load that binary before its own version-matched native asset.
Fix: Probe application-local and packaged runtime assets first. Restrict Cargo-layout probing to an explicit developer override or a verified repository root, preferably Debug-only.
| /// </summary> | ||
| /// <exception cref="MxcException">The exec could not be started.</exception> | ||
| public static MxcSandboxProcess ExecInSandbox(SandboxId id, string command) | ||
| public static MxcSandboxProcess ExecInSandbox( |
There was a problem hiding this comment.
High (backward compatibility) — Preserve the existing public method arities.
ExecInSandbox, ExecInSandboxAsync, StopSandbox, and DeprovisionSandbox replace existing signatures by appending optional parameters. Optional parameters preserve source compatibility only; already-compiled callers reference the old method signatures and will fail with MissingMethodException when the new 0.7.0 assembly is substituted.
Fix: Keep overloads with the original arities and forward them to new overloads accepting options, or ship this as a versioned breaking change.
| } | ||
|
|
||
| /// <summary>Request consent through an asynchronous host presenter.</summary> | ||
| public static Task<TelemetryConsentOutcome> RequestConsentAsync( |
There was a problem hiding this comment.
Medium (performance/reliability) — RequestConsentAsync is sync-over-async.
The implementation uses Task.Run around the synchronous FFI call, while the unmanaged presenter trampoline blocks that worker with GetAwaiter().GetResult(). This occupies a thread for the full human interaction and can deadlock when a caller blocks on the returned task while the presenter captures that caller's synchronization context.
Fix: Use a resumable native presenter protocol instead of blocking inside the unmanaged callback. At minimum accept cancellation and document that presenters must not require a captured context.
| } | ||
|
|
||
| /** Read persisted/effective consent and policy. */ | ||
| export function queryTelemetryConsent(): TelemetryConsentQuery { |
There was a problem hiding this comment.
Medium (performance) — Avoid repeated synchronous child processes for one status snapshot.
queryTelemetryConsent() uses execFileSync, and each convenience getter calls it independently. A settings surface reading consent, policy, and prompt-needed state launches three native processes and can block the Node event loop for up to three five-second timeouts.
Fix: Make the combined query the primary API and reuse one result, or cache a snapshot briefly and invalidate it after request/withdraw.
| ParsePolicyState(root.GetProperty("policy").GetString())); | ||
| } | ||
|
|
||
| private static TelemetryConsentActionResult ParseConsentActionResult(string? value) => value switch |
There was a problem hiding this comment.
Medium (compatibility/maintainability) — Extend parity checks to all consent wire mappings and fail closed on unknown values.
The new parity gate verifies policy strings only. Consent action results and status reasons remain hand-maintained switches with no Rust/C# gate, and unknown values throw JsonException rather than degrading safely like consent state and policy do. A version skew after a successful native write can therefore be reported as a write failure.
Fix: Generate or parity-check ConsentState, ConsentActionResult, and ConsentStatusReason as well; map unknown result/reason values to safe diagnostic outcomes instead of throwing.
|
|
||
| it('binds a synchronous presenter decision to the canonical prompt', async () => { | ||
| let observedLocale: string | undefined; | ||
| _setTelemetryConsentProtocolRunner(async (locale, presenter) => { |
There was a problem hiding this comment.
Medium (test coverage) — Exercise the real Node consent subprocess protocol.
These tests replace defaultConsentProtocolRunner, so they bypass child launch, line framing, fragmented stdout, challenge echo, stdin errors, timeout rearming, presenter/child close ordering, and teardown. The recent timeout fix is therefore not protected by this suite.
Fix: Run the real protocol driver against a controlled fake executor covering fragmented output, slow presenters, malformed responses, early child exit, timeout phases, and cleanup.
| /** | ||
| * Consent operation to perform. | ||
| */ | ||
| action: unknown; |
There was a problem hiding this comment.
Medium (generated contract) — Preserve the discriminator types in the generated request.
The generator emits action and command as unknown even though it emits the literal TelemetryConsentAction and TelemetryConsentCommand aliases directly above. This weakens the drift oracle for the two fields that distinguish maintenance input from execution config; the codegen gate passes because it faithfully reproduces the same weak output.
Fix: Correct the schema/emitter so these properties reference the generated literal types and add a conformance assertion for them.
| * (`wxc_common::ts_emit`). This is a drift oracle, not public API: it is never | ||
| * exported from the SDK. The conformance test asserts the hand-written public | ||
| * types in `../types.ts` still match these. CI gate: | ||
| * `scripts/versioning/check-sdk-types-codegen.js`. |
There was a problem hiding this comment.
Low (documentation) — This generated header names the wrong gate and regeneration command.
The actual artifact is checked by check-telemetry-consent-codegen.js and generated with --telemetry-consent-ts; the header instead points to the generic SDK-types gate and regenerates wire.ts.
Fix: Parameterize the generated banner per artifact so contributors can reproduce this file correctly.
| impl PolicyKeyGuard { | ||
| /// Creates the guard with no policy value set (the unmanaged default). | ||
| #[cfg(target_os = "windows")] | ||
| // Copyright (c) Microsoft Corporation. |
There was a problem hiding this comment.
Low (review integrity) — Remove the line-ending-only rewrites.
policy.rs, docs/telemetry/telemetry-policy.md, and run_telemetry_consent_smoke_test.ps1 contribute 890 added and 890 deleted lines but are content-identical when end-of-line differences are ignored. This obscures the functional diff and rewrites blame for privacy-sensitive code without changing behavior.
Fix: Restore the base line endings for these files and add an appropriate .gitattributes normalization rule to prevent recurrence.
c8415af to
2f8f372
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 58 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
src/core/wxc_common/src/telemetry/events.rs:119
- This always derives the error event's
sandbox_kindfromctx.backend, so callers that resolved a broad SDK request such asprocessorvmemit paired records with different attribution:MXC.Executionkeeps the requested kind, whileMXC.Errorreports the concrete backend. Pass the request-scoped sandbox kind through the error path as well so both events remain joinable and semantically consistent.
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxPolicy.cs:41 - The FFI contract identifies nested
telemetry.enabledas canonical andtelemetryEnabledonly as a legacy compatibility alias (src/ffi/mxc_ffi/src/lib.rs:31-35). Serializing the newly shipped .NET surface through that alias makes new clients depend on the compatibility path; retain the convenience property but project it onto the canonical nested section.
[JsonPropertyName("telemetryEnabled")]
public bool? TelemetryEnabled { get; set; }
sdk/node/src/telemetry.ts:376
- This catch handles every transport/parser failure, including a missing executable, timeout, and malformed child output, but labels all of them as
store-unreadable. That reason specifically describes consent-store state and is false for these failures; omit it here and let the diagnosticerrordistinguish transport failures.
src/mxc_telemetry/src/lib.rs:358 - This source-text assertion is self-fulfilling:
provider_sourceincludes this test, so the searchedstr8("mxc.sandbox_kind", sandbox_kind)literal still appears later in the file even if the productionMXC.Errorfield is removed. Bound the extracted event body before the test module or assert captured/decoded event fields instead.
.github/copilot-instructions.md:228 - This changes the reference to
docs/telemetry/telemetry-policy.md, but that file does not exist; the checked-in policy document isdocs/telemetry/telemetry-administrative-policy.md. Keep the existing filename unless this PR also renames the document and updates all remaining SDK links.
- `docs/telemetry/telemetry.md` — telemetry overview; `docs/telemetry/telemetry-consent-design.md` (Windows-only consent design and per-SDK surface) and `docs/telemetry/telemetry-policy.md` (the MDM / Group Policy ceiling)
.github/copilot-instructions.md:333
- The referenced
docs/telemetry/telemetry-policy.mdis absent; the repository currently containstelemetry-administrative-policy.md, which the Node.js and .NET README links also use. Point this convention at the existing document or include a coordinated rename.
- **Telemetry consent or policy changes** → update `docs/telemetry/telemetry-consent-design.md` and/or `docs/telemetry/telemetry-policy.md`, and keep `scripts/check-telemetry-policy-parity.js` green across all three bindings
| childStdout.on('data', (chunk: string) => { | ||
| if (!presenterActive) { | ||
| armIoTimeout(); | ||
| } | ||
| stdout += chunk; | ||
| const lines = stdout.split(/\r?\n/); | ||
| stdout = lines.pop() ?? ''; |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2da373b9-0a51-4aa4-a968-14f05f27b5c7
2f8f372 to
d537442
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 58 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
.github/copilot-instructions.md:333
- The referenced
docs/telemetry/telemetry-policy.mdfile is absent; the maintained document istelemetry-administrative-policy.md. This checklist would direct future telemetry changes away from the actual policy documentation.
- **Telemetry consent or policy changes** → update `docs/telemetry/telemetry-consent-design.md` and/or `docs/telemetry/telemetry-policy.md`, and keep `scripts/check-telemetry-policy-parity.js` green across all three bindings
sdk/node/src/telemetry.ts:193
- Every stdout chunk re-arms the timeout before a complete protocol line exists, while the partial line is appended to an unbounded string. A stuck or incompatible child can therefore emit an endless no-newline stream, preventing the timeout forever and growing memory without limit. Bound the frame buffer and treat only complete protocol frames as progress, or enforce an absolute child deadline.
src/core/wxc_common/src/telemetry/events.rs:119 log_errorderives this field only fromctx.backend, discarding the request-scoped sandbox kind already computed byemit_state_aware_event(and the one-shot helpers). For requests such as kindprocess/vmrouted towindows_sandbox, the pairedMXC.ExecutionandMXC.Errorevents therefore report differentmxc.sandbox_kindvalues. Pass the computed sandbox kind through tolog_errorso both events retain caller attribution.
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxPolicy.cs:41- This serializes the new SDK property through the FFI's legacy compatibility alias
telemetryEnabled, even though the native contract and all other surfaces define canonical telemetry astelemetry.enabled(mxc_ffi/src/lib.rs:31-35). KeepTelemetryEnabledas a convenience projection, but serialize the canonical nested section; otherwise the newly introduced public SDK starts on a deprecated wire shape and cannot expose future telemetry fields without another API break.
[JsonPropertyName("telemetryEnabled")]
public bool? TelemetryEnabled { get; set; }
sdk/node/src/telemetry.ts:304
- The parser validates shared fields but never validates the required
actiondiscriminator (or that the result is valid for that action). Consequently a status query can accept a well-formed response forrequest/withdrawand report its effective state—includinggranted—instead of failing closed on a mismatched native protocol. Pass the expected action into this parser and reject mismatched actions/results.
.github/copilot-instructions.md:228 - This path does not exist in the repository; the policy document is
docs/telemetry/telemetry-administrative-policy.md. As written, the repository guidance sends contributors to a missing file.
This issue also appears on line 333 of the same file.
- `docs/telemetry/telemetry.md` — telemetry overview; `docs/telemetry/telemetry-consent-design.md` (Windows-only consent design and per-SDK surface) and `docs/telemetry/telemetry-policy.md` (the MDM / Group Policy ceiling)
| <ProjectReference Include="..\Microsoft.Mxc.Sdk\Microsoft.Mxc.Sdk.csproj" | ||
| AdditionalProperties="MxcCargoFeatures=dotnetsdk,test-support" /> |
.github/copilot-instructions.md.Summary
Adds the Node.js and .NET telemetry surfaces over the reviewed native contract: presenter-driven consent, policy-aware status, withdrawal, per-run options for one-shot and state-aware execution, generated wire types, parity checks, tests, and final SDK/product documentation.
This is PR 5 of 5 and depends on the Rust SDK/C ABI in PR 4.
Stack
Review only this PR's diff; prerequisite behavior is in the PR above.