KEMBAR78
Remove the use of Windows PowerShell ETW provider id and update `PSDiagnostics` module to work for PowerShell 7 by daxian-dbw · Pull Request #25590 · PowerShell/PowerShell · GitHub
Skip to content

Remove the use of Windows PowerShell ETW provider id and update PSDiagnostics module to work for PowerShell 7 #25590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 1, 2025
Merged

Remove the use of Windows PowerShell ETW provider id and update PSDiagnostics module to work for PowerShell 7 #25590

merged 3 commits into from
Oct 1, 2025

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented May 21, 2025 •
edited
Loading

PR Summary

This PR includes 2 changes to clean up the ETW logging related code in PowerShell:

  1. Remove the use of Windows PowerShell ETW provider id (a0c1853b-5c40-4b15-8766-3cf1c58f985a) in current code base. This seems to be an oversight from the initial effort to create manifest based ETW provider for PowerShell 7+ (Redirect ETW logging to Syslog on Linux #5144).
  2. Minor updates to the PSDiagnostics module to make it work against PowerShell 7 -- enable/disable the PowerShellCore ETW provider instead of Microsoft-Windows-PowerShell, which is the ETW provider for Windows PowerShell.

PR Checklist

@daxian-dbw daxian-dbw mentioned this pull request May 21, 2025
Closed
2 tasks
@daxian-dbw daxian-dbw added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label May 21, 2025
jborean93
jborean93 reviewed May 22, 2025
View reviewed changes
Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

I'm not really too familiar with the ETW side and the PSDiagnostics module but wouldn't this be a breaking change? Right now the PSDiagnostics module is importable in PS 7 and manages the WinPS provider. While I understand trying to keep the providers aligned with the PowerShell version used there could potentially be people using PS 7 to manage the WinPS provider in this module.

src/System.Management.Automation/utils/tracing/TracingGen.cs
/// </summary>
public const long KeywordAll = 0xFFFFFFFF;

private static readonly Guid providerId = Guid.Parse("a0c1853b-5c40-4b15-8766-3cf1c58f985a");
Copy link
Collaborator

@jborean93 jborean93 May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason why this doesn't use the PowerShellCore Provider ID f90714a8-5509-434a-bf6d-b1624c8a19a2 rather than just removing it?

Copy link
Collaborator

@iSazonov iSazonov May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is already there. It is inherited from EwtActivity.ProviderId (that is PSEtwLogProvider.ProviderGuid).
The (whole) code looks a bit confusing, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an oversight when introducing the PowerShellCore provider initially in #5144.

The Tracer class is used in *-PSSessionConfiguration commands to write ETW logs like Endpoint registered/unregistered/modified/enabled/disabled. Today, those events are writting to the Windows PowerShell provider. The class is only used in ContainerParentJob.

@daxian-dbw daxian-dbw added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label May 22, 2025
@daxian-dbw
Copy link
Member Author

I'm not really too familiar with the ETW side and the PSDiagnostics module but wouldn't this be a breaking change? Right now the PSDiagnostics module is importable in PS 7 and manages the WinPS provider. While I understand trying to keep the providers aligned with the PowerShell version used there could potentially be people using PS 7 to manage the WinPS provider in this module.

The change to PSDiagnostics is a breaking change. It breaks the scenario where a user intentionally uses PowerShell 7+ to enable/disable Windows PowerShell ETW tracing. But is that a real-world scenario?

This module is shipped with PowerShell 7, so intuitively, running Enable-PSTrace from this module should enable the PowerShellCore provider instead of the Windows PowerShell ETW provider.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label May 30, 2025
@daxian-dbw daxian-dbw added WG-Cmdlets general cmdlet issues WG-NeedsReview Needs a review by the labeled Working Group labels Sep 22, 2025
SeeminglyScience
SeeminglyScience approved these changes Oct 1, 2025
View reviewed changes
Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM!

TravisEz13
TravisEz13 reviewed Oct 1, 2025
View reviewed changes
@daxian-dbw
Copy link
Member Author

/azp run PowerShell-CI-linux-packaging, PowerShell-Windows-Packaging-CI

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@daxian-dbw daxian-dbw merged commit b852378 into PowerShell:master Oct 1, 2025
36 checks passed
@daxian-dbw daxian-dbw deleted the provider-id branch October 1, 2025 19:38
@daxian-dbw daxian-dbw removed the Review - Needed The PR is being reviewed label Oct 1, 2025
@microsoft-github-policy-service
Copy link
Contributor

microsoft-github-policy-service bot commented Oct 1, 2025 •
edited by unfurl-links bot
Loading

📣 Hey @@daxian-dbw, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

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

Reviewers

@iSazonov iSazonov iSazonov left review comments

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

+1 more reviewer

@jborean93 jborean93 jborean93 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log WG-Cmdlets general cmdlet issues WG-NeedsReview Needs a review by the labeled Working Group

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@daxian-dbw @jborean93 @TravisEz13 @iSazonov @SeeminglyScience