Add EventPipe EventSource serialization testing #120887

noahfalk · 2025-10-19T23:36:13Z

We've had EventSource serialization tests for a long time but EventPipe never got added to it. Unforetunately not testing EventPipe in this way previously meant that quite a few discrepancies between EventPipe and ETW went unnoticed. As a first step to documenting and/or resolving the problems I wanted to get EventPipe running in the same set of tests. I also did several other small fixes and improvements while doing the work.

EventPipeListener added as a new 3rd target for EventSource (in addition to EventListener and ETW which are there now)
Modified the Listener test abstraction so that EventPipe could also work
- Removed assumption that providers can changed after the listener starts running
- Added capability flags to indicate whether listeners supported certain kinds of serialization behavior
- Worked around incomplete implementation of IDictionary for payloads in TraceEvent
Leveraged xunit theory attribute to avoid needing different top-level APIs for every listener configuration
Fixed EventTestHarness assert which had actual and expected values swapped
TestsWrite: Fixed mistaken call to Array.Equals with Assert.Equal so the test actually validates what it was supposed to
Removed ActiveIssue 88305 attribute which was already closed in the past.
Moved some test cases from TestsWriteEvent.Etw.cs to TestsWriteEvent.cs so that they can run for other listeners when supported.

We've had EventSource serialization tests for a long time but EventPipe never got added to it. Unforetunately not testing EventPipe in this way previously meant that quite a few discrepancies between EventPipe and ETW went unnoticed. As a first step to documenting and/or resolving the problems I wanted to get EventPipe running in the same set of tests. I also did several other small fixes and improvements while doing the work. - EventPipeListener added as a new 3rd target for EventSource (in addition to EventListener and ETW which are there now) - Modified the Listener test abstraction so that EventPipe could also work - Removed assumption that providers can changed after the listener starts running - Added capability flags to indicate whether listeners supported certain kinds of serialization behavior - Worked around incomplete implementation of IDictionary for payloads in TraceEvent - Leveraged xunit theory attribute to avoid needing different top-level APIs for every listener configuration - Fixed EventTestHarness assert which had actual and expected values swapped - TestsWrite: Fixed mistaken call to Array.Equals with Assert.Equal so the test actually validates what it was supposed to - Removed ActiveIssue 88305 attribute which was already closed in the past. - Moved some test cases from TestsWriteEvent.Etw.cs to TestsWriteEvent.cs so that they can run for other listeners when supported.

noahfalk · 2025-10-19T23:36:57Z

fyi @lateralusX

dotnet-policy-service · 2025-10-19T23:37:18Z

Tagging subscribers to this area: @tarekgh, @tommcdon, @steveisok
See info in area-owners.md if you want to be subscribed.

mdh1418

Do we only want to test DynamicConfigChange on particular tests? Or would it be valuable to have both DynamicConfigChange and non DynamicConfigChange for all EventSource tests? Just curious when to test Start -> EventSourceCommand vs EventSourceCommand -> Start like how TestsUserErrors.cs uses the former in Test_BadTypes_Manifest_UserClass but uses the latter in Test_BadEventSource_MismatchedIds.

nit: inconsistency of one-line if blocks using/not-using curly braces + if( -> if (

mdh1418 · 2025-10-20T15:19:47Z

eng/Versions.props

    <MicrosoftDiaSymReaderVersion>2.0.0</MicrosoftDiaSymReaderVersion>
    <MicrosoftDiaSymReaderNativeVersion>17.10.0-beta1.24272.1</MicrosoftDiaSymReaderNativeVersion>
    <TraceEventVersion>3.1.16</TraceEventVersion>
+    <MicrosoftDiagnosticsNetCoreClientVersion>0.2.621003</MicrosoftDiagnosticsNetCoreClientVersion>


This reminded me that I added a modified copy of Microsoft.Diagnostics.NETCore.Client in the runtime to get the EventPipe tests at src/tests/tracing/eventpipe working for Android #64358. Were there any changes on the diagnostics side that are missing in runtime's local copy that EventPipeListener needs?

I didn't run into anything but I also didn't run the tests on Android during my local testing. If loopback DiagnosticsClient doesn't work there I'd probably just turn the tests off on that platform. The goal of the new testing was to validate the serialization works and all the serialization code should be identical regardless of platform/OS.

I'm wondering if it's reasonable to still reference the runtime's copy src/tests/tracing/eventpipe/common/Microsoft.Diagnostics.NETCore.Client in System.Diagnostics.Tracing.Tests.csproj. I wasn't particularly thinking about the test running on Android, more that #64358 removed this property so all tests would use the runtime copy. Reintroducing this property means there's some places that uses the runtimes copy and other places that uses the diagnostics copy.

I'm fine if we want to do that, I just recall that one of the previous debts was that the runtime copy would be bumped if there was new logic in the diagnostics copy, so I was just curious if there was something in 0.2.621003 that the local copy was missing.

If we don't need to run the serialization tests on all platforms and we mainly need the runtime version of the NETCore.Client on mobile platforms using TCP/IP, then maybe we should keep using runtime version for mobile platforms and turn of the serialization tests on those platforms. Alternative is to bring over whats missing into the runtime version of NETCore.Client (and probably turn off the serialization tests on mobile if it depends on spawning additional processes).

mdh1418 · 2025-10-20T15:46:33Z

src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/Harness/Listeners.cs

        public void EventSourceSynchronousEnable(EventSource eventSource, FilteringOptions options = null)
        {
+            if(!IsDynamicConfigChangeSupported)


nit spacing

mdh1418 · 2025-10-20T16:17:27Z

src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestsWriteEvent.cs

-        /// </summary>
-        [Fact]
-        public void Test_WriteEvent_ComplexData_SelfDescribing_EventListener()
+


nit: extra line

noahfalk · 2025-10-20T21:25:12Z

Do we only want to test DynamicConfigChange on particular tests? Or would it be valuable to have both DynamicConfigChange and non DynamicConfigChange for all EventSource tests? Just curious when to test Start -> EventSourceCommand vs EventSourceCommand -> Start

I think there are a few parts to this and I believe they are still getting tested:

What is the ordering of enabling the provider in respect to enabling the session. ETW and EventListener are enabling the session first and providers afterwards as they were before. Although the EventSourceCommand() calls occur before Start() the internal queueing means the commands get executed after the session already started.
What is the ordering of enabling the provider relative to registering the EventSource - both orderings are tested.
What is the ordering of disabling the provider relative to ending the session. The test disables one of the providers early to confirm the events aren't received but this is only done for ETW and EventListener since EventPipe doesn't support the dynamic config change.

Copilot

Pull Request Overview

This PR adds EventPipe as a third event listener target alongside the existing EventListener and ETW implementations in the EventSource serialization tests. Previously, EventPipe was not tested through this framework, which allowed serialization discrepancies between EventPipe and ETW to go unnoticed.

Key Changes:

Added EventPipeListener implementation using DiagnosticsClient
Refactored test infrastructure to support multiple listener configurations via xunit Theory
Added capability flags for listener-specific serialization behaviors
Fixed several existing test issues and removed closed ActiveIssue attributes

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
System.Diagnostics.Tracing.Tests.csproj	Added EventPipeListener.cs and Microsoft.Diagnostics.NetCore.Client package reference
TestsWriteEventToListener.cs	Added listener.Start() calls to enable proper listener initialization
TestsWriteEvent.cs	Converted individual test methods to Theory-based approach using GetListenerConfigurations(), added conditional test execution based on listener capabilities
TestsWriteEvent.Etw.cs	Updated method signatures and removed ActiveIssue attributes for resolved issue #88305
TestsWrite.cs	Converted to Theory-based tests, fixed Array.Equals/Assert.Equal bug, added listener capability checks
TestsWrite.Etw.cs	Removed ActiveIssue attribute for resolved issue #88305
TestsUserErrors.cs	Added listener.Start() calls for proper initialization
Listeners.cs	Added Start() method, capability flags, pending command queue, and fixed IDictionary iteration bug
EventTestHarness.cs	Updated to use new Start() pattern and fixed swapped Assert.Equal arguments
EventPipeListener.cs	New EventPipeListener implementation for EventPipe event collection
EtwListener.cs	Refactored to support pending command queue and added Start() method
Versions.props	Added MicrosoftDiagnosticsNetCoreClientVersion property

lateralusX · 2025-10-21T08:55:10Z

src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestsWrite.cs

        {
-            using (var listener = new EventListenerListener())
+            TheoryData<Listener> data = new TheoryData<Listener>();
+            if (PlatformDetection.IsNetCore && PlatformDetection.IsNotAndroid && PlatformDetection.IsNotBrowser && PlatformDetection.IsNotMonoRuntime)


I guess EventPipe tests would work on Mono runtime desktop platforms, did it get excluded just because it was not tested or is there any know issues or limitations?

I excluded it because it appeared DiagnosticClient(int pid) wasn't working to self-attach. Does that sound right or maybe I misinterpreted something?

I assume it won't work on EventPipe using TCP/IP (Android, iOS, WebAssembly) since it won't create an IPC channel based on PID + no default diagnostic listener, but for Mono desktop/maccatalyst it uses the same IPC implementation as CoreCLR by default, so I assume it should work. If it doesn't work on Mono desktop/maccatalyst, then I believe we should look into why, would be great to at least have one Mono platform running these tests as well.

noahfalk requested review from Copilot and mdh1418 and removed request for Copilot October 19, 2025 23:36

dotnet-policy-service bot assigned noahfalk Oct 19, 2025

github-actions bot added the area-System.Diagnostics.Tracing label Oct 19, 2025

This was referenced Oct 20, 2025

[browser] Wasm.Build.Tests timeout - Timed out after 10s waiting for 'WASM EXIT' #116697

Open

Unable to pull image from mcr.microsoft.com #117164

Open

mdh1418 approved these changes Oct 20, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings October 20, 2025 22:41

Copilot AI reviewed Oct 20, 2025

View reviewed changes

Feedback + disabling platforms where EventPipe can't loopback

e83aebc

noahfalk force-pushed the eventpipe_metadata_tests branch from cb628f6 to e83aebc Compare October 20, 2025 22:56

lateralusX reviewed Oct 21, 2025

View reviewed changes

Add EventPipe EventSource serialization testing #120887

Are you sure you want to change the base?

Add EventPipe EventSource serialization testing #120887

Conversation

noahfalk commented Oct 19, 2025

Uh oh!

noahfalk commented Oct 19, 2025

Uh oh!

dotnet-policy-service bot commented Oct 19, 2025

Uh oh!

mdh1418 left a comment

Choose a reason for hiding this comment

Uh oh!

mdh1418 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

noahfalk Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

mdh1418 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

lateralusX Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

mdh1418 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

mdh1418 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

noahfalk commented Oct 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

lateralusX Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahfalk Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

lateralusX Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lateralusX Oct 21, 2025 •

edited

Loading

lateralusX Oct 21, 2025 •

edited

Loading