Introduce a new internal EventSourceGenerator and use it for all ES implementations #121180

EgorBo · 2025-10-29T18:13:45Z

Remove [EventSourceAutoGenerate], we rely on just [EventSource] + no explicit ctors
Introduce a new global SourceGenerators - EventSourceGenerator (and move the existing impl from SPC.Generators to this new project)
Update slnx files (via the UpdateSolutionFiles task)
Migrate all classes with [EventSource] to use the SG. Except for the ones with EventSourceSettings.EtwSelfDescribingEventFormat -- there are 3 ES impl defining this settings 🤔

EgorBo · 2025-10-29T18:15:22Z

hm.. probably should move the SLNX update to a separate (last) commit (or maybe even PR?)

…+ no explicit ctors 2) Introduce a new global SourceGenerators - EventSourceGenerator (and move the existing impl from SPC.Generators to this new project) 3) Update slnx files (via the UpdateSolutionFiles task) 4) Migrate all classes with [EventSource] to use the SG. Except for the ones with EtwSelfDescribingEventFormat

stephentoub · 2025-10-29T18:20:47Z

we rely on just [EventSource] + no explicit ctors

We will likely want more than just this, e.g. it'll need to be partial.

stephentoub · 2025-10-29T18:21:27Z

Update slnx files

Do we need to modify the slnx files? Could we instead globally include via .props?

EgorBo · 2025-10-29T18:31:39Z

Update slnx files

Do we need to modify the slnx files? Could we instead globally include via .props?

it's not needed to build the projects, but needed for people opening slnx files for the SG project to show up there in slnx. All other source generator projects are already added there (added automatically by the UpdateSolutionFile task, which is manually invoked (it's slow))

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Emitter.cs

src/libraries/System.Threading/src/System/Threading/CDSsyncETWBCLProvider.cs

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionsEtwProvider.cs

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Emitter.cs

Copilot

Pull Request Overview

This PR introduces a source generator for EventSource classes to automatically generate constructors and metadata, eliminating the need for manual boilerplate code. The generator creates parameterless constructors that call the base EventSource constructor with the correct parameter order (name, guid).

Key changes:

Introduces EventSourceGenerator source generator that triggers on classes with [EventSource] attribute
Removes [EventSourceAutoGenerate] attribute and makes affected EventSource classes partial
Changes EventSource base constructor parameter order from (Guid, string) to (string, Guid) for consistency
Removes manual constructors from EventSource classes across the codebase

Reviewed Changes

Copilot reviewed 50 out of 50 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
EventSourceGenerator.cs	New source generator that creates constructors for EventSource classes
EventSourceGenerator.Parser.cs	Parser logic to extract EventSource metadata and validate constructors
EventSourceGenerator.Emitter.cs	Code generation logic for constructors and provider metadata
EventSource.cs	Changes constructor parameter order and visibility for source generator
TraceLoggingEventSource.cs	Updates constructor call to match new parameter order
Multiple EventSource classes	Removes manual constructors and adds `partial` keyword
NetCoreAppLibrary.props	Registers EventSourceGenerator
generators.targets	Enables generator for source projects
Directory.Build.props	Suppresses ESGEN001 warnings globally

Comments suppressed due to low confidence (1)

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs:24

The parser only checks for NamespaceDeclarationSyntax but doesn't check for FileScopedNamespaceDeclarationSyntax. This will cause the generator to skip classes in file-scoped namespaces. Use BaseNamespaceDeclarationSyntax as the base type to handle both traditional and file-scoped namespace declarations.

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Emitter.cs

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs

src/libraries/System.Net.Http/src/System/Net/Http/HttpTelemetry.cs

...es/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs

src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs

src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs

...es/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceEventSource.cs

jkotas

Delete redundant comments

...stem.Collections.Concurrent/src/System/Collections/Concurrent/CDSCollectionETWBCLProvider.cs

src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Utils/PLINQETWProvider.cs

src/libraries/System.Threading.Tasks.Dataflow/src/Internal/DataflowEtwProvider.cs

src/libraries/System.Threading/src/System/Threading/CDSsyncETWBCLProvider.cs

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionsEtwProvider.cs

jkotas

Unused?

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/RuntimeEventSource.cs

…acing/RuntimeEventSource.cs

jkotas · 2025-11-10T02:40:54Z

src/libraries/System.Diagnostics.Tracing/ref/System.Diagnostics.Tracing.cs

        public EventSource(string eventSourceName, System.Diagnostics.Tracing.EventSourceSettings config) { }
        public EventSource(string eventSourceName, System.Diagnostics.Tracing.EventSourceSettings config, params string[]? traits) { }
+        public EventSource(string eventSourceName, Guid eventSourceGuid) { }
+        public EventSource(string eventSourceName, Guid eventSourceGuid, System.Diagnostics.Tracing.EventSourceSettings settings, params string[]? traits) { }


The approved API shape (and the implementation) has default null value for traits, but this has params and no null value. Which one is correct?

Do we need to add a test to validate that the new constructor works as expected?

@jkotas sorry, I am not following - what that test is expected to cover?

PS: added = null and removed params to match exactly the approved shape. Presumably, this particular API is mostly for the SG.

Although, isn't params string[] x and string[] x = null are the same thing? 🤔
I guess adding params (but not removing) shouldn't be a breaking change. However, feels like collections literals effectively made params redundant

Although, isn't params string[] x and string[] x = null are the same thing?

No, if you don't pass an argument for the latter, x will be null, but if you don't pass an argument for the former, x will be Array.Empty<string>().

what that test is expected to cover?

The tests for the existing APIs are in https://github.com/dotnet/runtime/blob/5bceb8184fcde55143d2102a65c72bd6233e950b/src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestsTraits.cs and related files.

Add some tests like that for the new APIs?

src/libraries/System.Diagnostics.Tracing/ref/System.Diagnostics.Tracing.cs

…s.Tracing.cs

jkotas · 2025-11-10T23:25:39Z

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

            }
        }

        // FrameworkEventSource is on the startup path for the framework, so we have this internal overload that it can use


This comment should be deleted and replaced with tripple slash comment that will be used to populate the docs.

jkotas · 2025-11-10T23:25:54Z

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

        { }

        // Used by the internal FrameworkEventSource constructor and the TraceLogging-style event source constructor
-        internal EventSource(Guid eventSourceGuid, string eventSourceName, EventSourceSettings settings, string[]? traits = null)


EgorBo · 2025-11-11T00:21:39Z

@copilot address the recent feedback

EgorBo · 2025-11-11T00:22:14Z

oh, that doesn't work that way?

jkotas · 2025-11-11T00:38:14Z

oh, that doesn't work that way?

You can ask it to create a PR against your branch, but it will never do a direct push into your branch currently.

github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 29, 2025

EgorBo changed the title ~~Es cleanup~~ Introduce a new internal EventSourceGenerator and use it for all ES implementations Oct 29, 2025

EgorBo force-pushed the ES-cleanup branch from ae857c0 to 21f579b Compare October 29, 2025 18:19

jkotas reviewed Oct 29, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs Show resolved Hide resolved

EgorBo added 4 commits October 29, 2025 23:15

Merge branch 'main' into ES-cleanup

c4cf897

fix build

f5dbdd2

Merge branch 'ES-cleanup' of github.com:EgorBo/runtime-1 into ES-cleanup

c702892

cleanup

62433fe

This was referenced Oct 30, 2025

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

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

Open

[QUIC] Sporadic long running tests #119763

Open

EgorBo mentioned this pull request Oct 30, 2025

[API Proposal]: Settings property in EventSourceAttribute #121205

Open

am11 reviewed Oct 30, 2025

View reviewed changes

jkotas reviewed Oct 31, 2025

View reviewed changes

src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Emitter.cs Outdated Show resolved Hide resolved

Copilot AI mentioned this pull request Nov 1, 2025

Add performance and compatibility details to EtwManifestEventFormat and EtwSelfDescribingEventFormat docs dotnet/dotnet-api-docs#12000

Merged

EgorBo added 2 commits November 4, 2025 16:42

Merge branch 'main' of https://github.com/dotnet/runtime into ES-cleanup

637e6c1

Address feedback

4ccefa9

This was referenced Nov 4, 2025

[wasm/browser] Failed to connect to socket /run/dbus/system_bus_socket #120176

Open

System.Security.Cryptography.X509Certificates.Tests fails on Linux #120527

Open

Introduce a warning

20d7953

EgorBo marked this pull request as ready for review November 4, 2025 22:45

Copilot AI review requested due to automatic review settings November 4, 2025 22:45

Copilot AI reviewed Nov 4, 2025

View reviewed changes

EgorBo added 2 commits November 4, 2025 23:59

feedback

2f30603

Merge branch 'main' of https://github.com/dotnet/runtime into ES-cleanup

e97c786

Helen-con approved these changes Nov 9, 2025

View reviewed changes

EgorBo added 2 commits November 9, 2025 19:24

Merge branch 'main' of https://github.com/dotnet/runtime into ES-cleanup

4f2b250

feedback

6e8f918