Update PowerShell telemetry to respect the diagnostics and feedback setting on Windows#27328
Conversation
…etting on Windows
There was a problem hiding this comment.
Pull request overview
Updates PowerShell’s telemetry initialization on Windows to respect the OS “Diagnostics & feedback” policy, and adds ETW plumbing to surface failures when reading that policy.
Changes:
- Add
WindowsDataCollectionSettingWinRT/COM interop helper to queryPlatformDiagnosticsAndUsageDataSettings. - Gate
ApplicationInsightsTelemetryinitialization based on Windows diagnostics collection level. - Add a new operational ETW event/task and update the ETW manifest/resources accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/System.Management.Automation/utils/WindowsDataCollectionSetting.cs |
New WinRT activation + CanCollectDiagnostics helper with ETW error logging on failure. |
src/System.Management.Automation/utils/Telemetry.cs |
Telemetry enablement now depends on OS diagnostics policy (Windows) in addition to existing opt-out + UUID checks. |
src/System.Management.Automation/engine/remoting/common/PSETWTracer.cs |
Adds Telemetry_Setting_Error event id and Telemetry task enum value used by the new ETW log. |
src/PowerShell.Core.Instrumentation/PowerShell.Core.Instrumentation.man |
Adds manifest entries/resources for the new telemetry setting error event/task (and related convergence updates). |
| // - S_OK (0) means we initialized for the first time. | ||
| // - S_FALSE (1) means already initialized, but still increments the reference count. | ||
| // Both require CoUninitialize to decrement the reference count. | ||
| if (result >= 0) |
There was a problem hiding this comment.
This fix is not related to the feature. I found it when reading the code. The requirement for balancing the call of CoInitializeEx is documented in https://learn.microsoft.com/en-us/windows/win32/api/combaseapi/nf-combaseapi-coinitializeex#remarks. I quote it below:
To uninitialize the COM library gracefully on a thread, each successful call to CoInitialize or CoInitializeEx, including any call that returns S_FALSE, must be balanced by a corresponding call to CoUninitialize.
There was a problem hiding this comment.
This fix is not related to the feature. I found it when reading the code. The requirement for balancing the call of
CoInitializeExis documented in (...)
I wonder if anyone is (incorrectly ofc) depending on us having initialized COM. Not suggesting the change be reverted, but worth keeping in mind if it comes up.
| // - S_OK (0) means we initialized for the first time. | ||
| // - S_FALSE (1) means already initialized, but still increments the reference count. | ||
| // Both require CoUninitialize to decrement the reference count. | ||
| if (result >= 0) |
There was a problem hiding this comment.
This fix is not related to the feature. I found it when reading the code. The requirement for balancing the call of
CoInitializeExis documented in (...)
I wonder if anyone is (incorrectly ofc) depending on us having initialized COM. Not suggesting the change be reverted, but worth keeping in mind if it comes up.
PR Summary
Update PowerShell telemetry to respect the diagnostics and feedback setting on Windows.
WindowsDataCollectionSetting.csto handle calling the WinRT APIIPlatformDiagnosticsAndUsageDataSettingsStaticsfalseto disable the telemetryPowerShell.Core.Instrumentation.manto include the resources used in the new ETW event.PowerShellrepo and thePowerShell-Nativerepo, and actually, the one inPowerShell-Nativeis what we use to create the resource DLLPowerShell.Core.Instrumentation.dll.PowerShell.Core.Instrumentation.manfile to converge the changes in both placesPowerShell-NativerepoPR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header