PR feedback

src/libraries/System.Diagnostics.DiagnosticSource/src/ILLink/ILLink.Descriptors.LibraryBuild.xml

@@ -1,7 +1,7 @@
 <linker>
   <assembly fullname="System.Diagnostics.DiagnosticSource">
     <type fullname="System.Diagnostics.Metrics.MetricsEventSource">
-      <!-- Used by S.P.C via reflection to init the EventSource -->
+      <!-- Used by System.Private.CoreLib via reflection to init the EventSource -->
       <method name="GetInstance" />
     </type>
   </assembly>

src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj

@@ -25,6 +25,7 @@ System.Diagnostics.DiagnosticSource</PackageDescription>
 
   <ItemGroup>
     <ILLinkSubstitutionsXmls Include="ILLink/ILLink.Substitutions.Shared.xml" />
+    <Content Include="ILLink\ILLink.Descriptors.LibraryBuild.xml" />
   </ItemGroup>
 
   <ItemGroup>
@@ -85,7 +86,6 @@ System.Diagnostics.DiagnosticSource</PackageDescription>
     <Compile Include="$(CommonPath)Internal\Padding.cs" Link="Common\Internal\Padding.cs" />
     <Compile Include="$(CommonPath)System\HexConverter.cs" Link="Common\System\HexConverter.cs" />
     <Compile Include="$(CommonPath)System\Diagnostics\DiagnosticsHelper.cs" Link="Common\System\Diagnostics\DiagnosticsHelper.cs" />
-    <Content Include="ILLink\ILLink.Descriptors.LibraryBuild.xml" />
 
     <None Include="DiagnosticSourceUsersGuide.md" />
     <None Include="ActivityUserGuide.md" />

src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/MetricsEventSource.cs

@@ -47,8 +47,8 @@ internal sealed class MetricsEventSource : EventSource
         public static readonly MetricsEventSource Log = new();
 
         // Although this API isn't public, it is invoked via reflection from System.Private.CoreLib and needs the same back-compat
-        // consideration as a public API. See EventSource.InitializeDefaultEventSources() in S.P.C source for more details.
-        // We have a unit test GetInstanceMethodIsReflectable that verifies this method isn't accidentally removed or renamed.
+        // consideration as a public API. See EventSource.InitializeDefaultEventSources() in System.Private.CoreLib source for more
+        // details. We have a unit test GetInstanceMethodIsReflectable that verifies this method isn't accidentally removed or renamed.
         public static MetricsEventSource GetInstance() { return Log; }
 
         private const string SharedSessionId = "SHARED";

src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricEventSourceTests.cs

@@ -30,8 +30,8 @@ public MetricEventSourceTests(ITestOutputHelper output)
         [Fact]
         public void GetInstanceMethodIsReflectable()
         {
-            // The startup code in S.P.C needs to be able to get the MetricsEventSource instance via reflection. See EventSource.InitializeDefaultEventSources() in
-            // the S.P.C source.
+            // The startup code in System.Private.CoreLib needs to be able to get the MetricsEventSource instance via reflection. See EventSource.InitializeDefaultEventSources() in
+            // the System.Private.CoreLib source.
             // Even though the the type isn't public this test ensures the GetInstance() API isn't removed or renamed.
             Type? metricsEventSourceType = Type.GetType("System.Diagnostics.Metrics.MetricsEventSource, System.Diagnostics.DiagnosticSource", throwOnError: false);
             Assert.True(metricsEventSourceType != null, "Unable to get MetricsEventSource type via reflection");

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

@@ -2407,19 +2407,19 @@ private sealed class OverrideEventProvider : EventProvider
             public OverrideEventProvider(Func<EventSource?> eventSourceFactory, EventProviderType providerType)
                 : base(providerType)
             {
-                this.m_eventSourceFactory = eventSourceFactory;
-                this.m_eventProviderType = providerType;
+                _eventSourceFactory = eventSourceFactory;
+                _eventProviderType = providerType;
             }
             internal override void OnControllerCommand(ControllerCommand command, IDictionary<string, string?>? arguments,
                                                               int perEventSourceSessionId)
             {
                 // We use null to represent the ETW EventListener.
                 EventListener? listener = null;
-                m_eventSourceFactory()?.SendCommand(listener, m_eventProviderType, perEventSourceSessionId,
+                _eventSourceFactory()?.SendCommand(listener, _eventProviderType, perEventSourceSessionId,
                                           (EventCommand)command, IsEnabled(), Level, MatchAnyKeyword, arguments);
             }
-            private readonly Func<EventSource?> m_eventSourceFactory;
-            private readonly EventProviderType m_eventProviderType;
+            private readonly Func<EventSource?> _eventSourceFactory;
+            private readonly EventProviderType _eventProviderType;
         }
 
         /// <summary>
@@ -3833,7 +3833,7 @@ internal static void InitializeDefaultEventSources()
             // pulling the System.Diagnostics.DiagnosticSource assembly into the process until it is needed.
             if (AppContext.TryGetSwitch("System.Diagnostics.Metrics.Meter.IsSupported", out bool isSupported) ? isSupported : true)
             {
-                string name = "System.Diagnostics.Metrics";
+                const string name = "System.Diagnostics.Metrics";
                 Guid id = new Guid("20752bc4-c151-50f5-f27b-df92d8af5a61");
                 PreregisterEventProviders(id, name, GetMetricsEventSource);
             }
@@ -3869,6 +3869,10 @@ private static unsafe void PreregisterEventProviders(Guid id, string name, Func<
             //    traits but the SetInformation() call we use below generates metadata from name only. If we want traits support
             //    in the future it could be added.
 
+            // NOTE: You might think this preregister logic could be simplified by using an Action to create the EventSource instead of
+            // Func<EventSource> and then allow the EventSource to initialize as normal. This doesn't work however because calling
+            // EtwEventProvider.Register() inside of an ETW callback deadlocks. Instead we have to bind the EventSource to the
+            // EtwEventProvider that was already registered and use the callback we got on that provider to invoke EventSource.SendCommand().
             try
             {
                 s_preregisteredEventSourceFactories.Add(eventSourceFactory);
@@ -3936,26 +3940,24 @@ internal static void EnsurePreregisteredEventSourcesExist()
         {
             lock (s_preregisteredEtwProviders)
             {
-                s_preregisteredEtwProviders.TryGetValue(id, out OverrideEventProvider? provider);
-                s_preregisteredEtwProviders.Remove(id);
+                s_preregisteredEtwProviders.Remove(id, out OverrideEventProvider? provider);
                 return provider;
             }
         }
 
-        private static Dictionary<Guid, OverrideEventProvider> s_preregisteredEtwProviders = new Dictionary<Guid, OverrideEventProvider>();
+        private static readonly Dictionary<Guid, OverrideEventProvider> s_preregisteredEtwProviders = new Dictionary<Guid, OverrideEventProvider>();
 
 #if FEATURE_PERFTRACING
         private static OverrideEventProvider? TryGetPreregisteredEventPipeProvider(string name)
         {
             lock (s_preregisteredEventPipeProviders)
             {
-                s_preregisteredEventPipeProviders.TryGetValue(name, out OverrideEventProvider? provider);
-                s_preregisteredEventPipeProviders.Remove(name);
+                s_preregisteredEventPipeProviders.Remove(name, out OverrideEventProvider? provider);
                 return provider;
             }
         }
 
-        private static Dictionary<string, OverrideEventProvider> s_preregisteredEventPipeProviders = new Dictionary<string, OverrideEventProvider>();
+        private static readonly Dictionary<string, OverrideEventProvider> s_preregisteredEventPipeProviders = new Dictionary<string, OverrideEventProvider>();
 #endif
 
         // private instance state