PR feedback

wiktork · wiktork · commit b58beec9bc28 · 2025-05-02T14:15:30.000-07:00
diff --git a/documentation/schema.json b/documentation/schema.json
@@ -1123,6 +1123,14 @@
               "$ref": "#/definitions/ExceptionsConfiguration"
             }
           ]
+        },
+        "CollectOnStartup": {
+          "type": [
+            "boolean",
+            "null"
+          ],
+          "description": "True if exception collection should begin immediately",
+          "default": true
         }
       }
     },
diff --git a/src/Microsoft.Diagnostics.Monitoring.StartupHook/DiagnosticsBootstrapper.cs b/src/Microsoft.Diagnostics.Monitoring.StartupHook/DiagnosticsBootstrapper.cs
@@ -40,13 +40,13 @@ public DiagnosticsBootstrapper()
                         new MessageDispatcher.ProfilerMessageSource(CommandSet.StartupHook));
                     ToolIdentifiers.EnableEnvVar(InProcessFeaturesIdentifiers.EnvironmentVariables.AvailableInfrastructure.ManagedMessaging);
 
-                    SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.Stop, (IpcMessage) =>
+                    SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.StopAllFeatures, (IpcMessage) =>
                     {
                         _exceptionProcessor.Stop();
                         _parameterCapturingService?.RequestStopAll();
                     });
 
-                    SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.Start, (IpcMessage) =>
+                    SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.StartAllFeatures, (IpcMessage) =>
                     {
                         _exceptionProcessor.Start();
                     });
diff --git a/src/Microsoft.Diagnostics.Monitoring.StartupHook/Exceptions/CurrentAppDomainFirstChanceExceptionSource.cs b/src/Microsoft.Diagnostics.Monitoring.StartupHook/Exceptions/CurrentAppDomainFirstChanceExceptionSource.cs
@@ -15,10 +15,6 @@ internal sealed class CurrentAppDomainFirstChanceExceptionSource :
     {
         private long _disposedState;
 
-        public CurrentAppDomainFirstChanceExceptionSource()
-        {
-        }
-
         public override void Start()
         {
             AppDomain.CurrentDomain.FirstChanceException += CurrentDomain_FirstChanceException;
diff --git a/src/Microsoft.Diagnostics.Monitoring.StartupHook/Exceptions/CurrentAppDomainUnhandledExceptionSource.cs b/src/Microsoft.Diagnostics.Monitoring.StartupHook/Exceptions/CurrentAppDomainUnhandledExceptionSource.cs
@@ -14,10 +14,6 @@ internal sealed class CurrentAppDomainUnhandledExceptionSource :
     {
         private long _disposedState;
 
-        public CurrentAppDomainUnhandledExceptionSource()
-        {
-        }
-
         public override void Start()
         {
             AppDomain.CurrentDomain.UnhandledException += CurrentDomain_UnhandledException;
diff --git a/src/Microsoft.Diagnostics.Monitoring.WebApi/ProfilerMessage.cs b/src/Microsoft.Diagnostics.Monitoring.WebApi/ProfilerMessage.cs
@@ -27,16 +27,16 @@ public enum ServerResponseCommand : ushort
     public enum ProfilerCommand : ushort
     {
         Callstack,
-        Stop,
-        Start,
+        StopAllFeatures,
+        StartAllFeatures,
     };
 
     public enum StartupHookCommand : ushort
     {
         StartCapturingParameters,
         StopCapturingParameters,
-        Stop,
-        Start,
+        StopAllFeatures,
+        StartAllFeatures,
     };
 
     public interface IProfilerMessage
diff --git a/src/Profilers/MonitorProfiler/Communication/CommandServer.cpp b/src/Profilers/MonitorProfiler/Communication/CommandServer.cpp
@@ -151,7 +151,7 @@ void CommandServer::ProcessResetMessage(const IpcMessage& message, std::shared_p
 
         _unmanagedOnlyQueue.Enqueue(nativeCallbackInfo);
 
-        hr = nativeCallbackInfo.Promise->get_future().get();
+        hr = nativeCallbackInfo.CompletionPromise->get_future().get();
     }
     if (message.CommandSet == static_cast<unsigned short>(CommandSet::StartupHook))
     {
@@ -161,7 +161,7 @@ void CommandServer::ProcessResetMessage(const IpcMessage& message, std::shared_p
 
         _clientQueue.Enqueue(managedCallbackInfo);
 
-        hr = managedCallbackInfo.Promise->get_future().get();
+        hr = managedCallbackInfo.CompletionPromise->get_future().get();
     }
 
     *reinterpret_cast<HRESULT*>(response.Payload.data()) = hr;
@@ -175,16 +175,16 @@ bool CommandServer::IsControlCommand(const IpcMessage& message)
         case static_cast<int>(CommandSet::Profiler):
             switch (message.Command)
             {
-                case static_cast<int>(ProfilerCommand::Start):
-                case static_cast<int>(ProfilerCommand::Stop):
+                case static_cast<int>(ProfilerCommand::StartAllFeatures):
+                case static_cast<int>(ProfilerCommand::StopAllFeatures):
                     return true;
                 default:
                     return false;
             }
         case static_cast<int>(CommandSet::StartupHook):
             switch (message.Command) {
-                case static_cast<int>(StartupHookCommand::Start):
-                case static_cast<int>(StartupHookCommand::Stop):
+                case static_cast<int>(StartupHookCommand::StartAllFeatures):
+                case static_cast<int>(StartupHookCommand::StopAllFeatures):
                     return true;
                 default:
                     return false;
@@ -250,9 +250,9 @@ void CommandServer::ProcessingThread(BlockingQueue<CallbackInfo>& queue)
             _logger->Log(LogLevel::Warning, _LS("IpcMessage callback failed: 0x%08x"), hr);
         }
 
-        if (info.Promise)
+        if (info.CompletionPromise)
         {
-            info.Promise->set_value(hr);
+            info.CompletionPromise->set_value(hr);
         }
     }
 }
diff --git a/src/Profilers/MonitorProfiler/Communication/CommandServer.h b/src/Profilers/MonitorProfiler/Communication/CommandServer.h
@@ -32,7 +32,7 @@ class CommandServer final
     {
         public:
             IpcMessage Message;
-            std::shared_ptr<std::promise<HRESULT>> Promise;
+            std::shared_ptr<std::promise<HRESULT>> CompletionPromise;
     };
 
     void ListeningThread();
@@ -48,7 +48,7 @@ class CommandServer final
         // Currently the managed payload always uses json deserialization
         // The native payload ignores this
         info.Message.Payload = std::vector<BYTE>({ (BYTE)'{', (BYTE)'}' });
-        info.Promise = std::make_shared<std::promise<HRESULT>>();
+        info.CompletionPromise = std::make_shared<std::promise<HRESULT>>();
     }
 
     // Wrapper methods for sending and logging
diff --git a/src/Profilers/MonitorProfiler/Communication/Messages.h b/src/Profilers/MonitorProfiler/Communication/Messages.h
@@ -11,11 +11,11 @@ enum class ProfilerCommand : unsigned short
 
     // Indicate that any outstanding collection should be stopped and all data should be flushed
     // Currently a no-op
-    Stop,
+    StopAllFeatures,
 
     // Indicate that collection should resume again
     // Currently a no-op
-    Start,
+    StartAllFeatures,
 };
 
 enum class StartupHookCommand : unsigned short
@@ -26,11 +26,11 @@ enum class StartupHookCommand : unsigned short
     // Indicates that all collection should stop
     // Request cancellation on all outstanding ParameterCapture requests
     // Unhooks from exception handling events, drains all the exception state, and resets the pipelines
-    Stop,
+    StopAllFeatures,
 
     // Indicates that all collection should start
     // Reconnects exception handler events
-    Start,
+    StartAllFeatures,
 };
 
 enum class ServerResponseCommand : unsigned short
diff --git a/src/Profilers/MonitorProfiler/MainProfiler/MainProfiler.cpp b/src/Profilers/MonitorProfiler/MainProfiler/MainProfiler.cpp
@@ -283,8 +283,8 @@ HRESULT MainProfiler::ProfilerCommandSetCallback(const IpcMessage& message)
     {
     case ProfilerCommand::Callstack:
         return ProcessCallstackMessage();
-    case ProfilerCommand::Start:
-    case ProfilerCommand::Stop:
+    case ProfilerCommand::StartAllFeatures:
+    case ProfilerCommand::StopAllFeatures:
         // TODO We don't do anything here now, but we could interrupt the current stack walk with CORPROF_E_STACKSNAPSHOT_ABORT in the snapshot callback.
         return S_OK;
     default:
diff --git a/src/Tools/dotnet-monitor/Profiler/ProfilerService.cs b/src/Tools/dotnet-monitor/Profiler/ProfilerService.cs
@@ -84,25 +84,25 @@ public async Task ApplyProfilersAsync(IEndpointInfo endpointInfo, CancellationTo
 
                     // Currently this is a no-op but ideally will interrupt all callstack collections
                     await _profilerChannel.SendMessage(endpointInfo,
-                        new JsonProfilerMessage((ushort)CommandSet.Profiler, (ushort)ProfilerCommand.Stop, new EmptyPayload()),
+                        new JsonProfilerMessage((ushort)CommandSet.Profiler, (ushort)ProfilerCommand.StopAllFeatures, new EmptyPayload()),
                         cancellationToken, ResetTimeout);
                     await _profilerChannel.SendMessage(endpointInfo,
-                        new JsonProfilerMessage((ushort)CommandSet.Profiler, (ushort)ProfilerCommand.Start, new EmptyPayload()),
+                        new JsonProfilerMessage((ushort)CommandSet.Profiler, (ushort)ProfilerCommand.StartAllFeatures, new EmptyPayload()),
                         cancellationToken, ResetTimeout);
 
                     if (_inProcessFeatures.IsStartupHookRequired)
                     {
                         // This will stop all exception pipelines from collecting data and request all parameter captures to be uninstrumented
                         await _profilerChannel.SendMessage(endpointInfo,
-                            new JsonProfilerMessage((ushort)CommandSet.StartupHook, (ushort)StartupHookCommand.Stop, new EmptyPayload()),
+                            new JsonProfilerMessage((ushort)CommandSet.StartupHook, (ushort)StartupHookCommand.StopAllFeatures, new EmptyPayload()),
                             cancellationToken, ResetTimeout);
 
                         //CONSIDER Currently this is granular enough because only exceptions really need starts. If that changes, we will need to be more verbose about
                         // which features need to be started rather than 1 stop/start for the entire command set.
                         if (_inProcessFeatures.CollectExceptionsOnStartup)
                         {
                             await _profilerChannel.SendMessage(endpointInfo,
-                                new JsonProfilerMessage((ushort)CommandSet.StartupHook, (ushort)StartupHookCommand.Start, new EmptyPayload()),
+                                new JsonProfilerMessage((ushort)CommandSet.StartupHook, (ushort)StartupHookCommand.StartAllFeatures, new EmptyPayload()),
                                 cancellationToken, ResetTimeout);
                         }
                     }

Original file line number	Diff line number	Diff line change
`@@ -1123,6 +1123,14 @@`
`1123`	`1123`	`"$ref": "#/definitions/ExceptionsConfiguration"`
`1124`	`1124`	`}`
`1125`	`1125`	`]`
	`1126`	`+ },`
	`1127`	`+ "CollectOnStartup": {`
	`1128`	`+ "type": [`
	`1129`	`+ "boolean",`
	`1130`	`+ "null"`
	`1131`	`+ ],`
	`1132`	`+ "description": "True if exception collection should begin immediately",`
	`1133`	`+ "default": true`
`1126`	`1134`	`}`
`1127`	`1135`	`}`
`1128`	`1136`	`},`
Original file line number	Diff line number	Diff line change
`@@ -40,13 +40,13 @@ public DiagnosticsBootstrapper()`
`40`	`40`	`new MessageDispatcher.ProfilerMessageSource(CommandSet.StartupHook));`
`41`	`41`	`ToolIdentifiers.EnableEnvVar(InProcessFeaturesIdentifiers.EnvironmentVariables.AvailableInfrastructure.ManagedMessaging);`
`42`	`42`
`43`		`- SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.Stop, (IpcMessage) =>`
	`43`	`+ SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.StopAllFeatures, (IpcMessage) =>`
`44`	`44`	`{`
`45`	`45`	`_exceptionProcessor.Stop();`
`46`	`46`	`_parameterCapturingService?.RequestStopAll();`
`47`	`47`	`});`
`48`	`48`
`49`		`- SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.Start, (IpcMessage) =>`
	`49`	`+ SharedInternals.MessageDispatcher.RegisterCallback<EmptyPayload>(StartupHookCommand.StartAllFeatures, (IpcMessage) =>`
`50`	`50`	`{`
`51`	`51`	`_exceptionProcessor.Start();`
`52`	`52`	`});`
Original file line number	Diff line number	Diff line change
`@@ -15,10 +15,6 @@ internal sealed class CurrentAppDomainFirstChanceExceptionSource :`
`15`	`15`	`{`
`16`	`16`	`private long _disposedState;`
`17`	`17`
`18`		`- public CurrentAppDomainFirstChanceExceptionSource()`
`19`		`- {`
`20`		`- }`
`21`		`-`
`22`	`18`	`public override void Start()`
`23`	`19`	`{`
`24`	`20`	`AppDomain.CurrentDomain.FirstChanceException += CurrentDomain_FirstChanceException;`
Original file line number	Diff line number	Diff line change
`@@ -14,10 +14,6 @@ internal sealed class CurrentAppDomainUnhandledExceptionSource :`
`14`	`14`	`{`
`15`	`15`	`private long _disposedState;`
`16`	`16`
`17`		`- public CurrentAppDomainUnhandledExceptionSource()`
`18`		`- {`
`19`		`- }`
`20`		`-`
`21`	`17`	`public override void Start()`
`22`	`18`	`{`
`23`	`19`	`AppDomain.CurrentDomain.UnhandledException += CurrentDomain_UnhandledException;`
Original file line number	Diff line number	Diff line change
`@@ -151,7 +151,7 @@ void CommandServer::ProcessResetMessage(const IpcMessage& message, std::shared_p`
`151`	`151`
`152`	`152`	`_unmanagedOnlyQueue.Enqueue(nativeCallbackInfo);`
`153`	`153`
`154`		`- hr = nativeCallbackInfo.Promise->get_future().get();`
	`154`	`+ hr = nativeCallbackInfo.CompletionPromise->get_future().get();`
`155`	`155`	`}`
`156`	`156`	`if (message.CommandSet == static_cast<unsigned short>(CommandSet::StartupHook))`
`157`	`157`	`{`
`@@ -161,7 +161,7 @@ void CommandServer::ProcessResetMessage(const IpcMessage& message, std::shared_p`
`161`	`161`
`162`	`162`	`_clientQueue.Enqueue(managedCallbackInfo);`
`163`	`163`
`164`		`- hr = managedCallbackInfo.Promise->get_future().get();`
	`164`	`+ hr = managedCallbackInfo.CompletionPromise->get_future().get();`
`165`	`165`	`}`
`166`	`166`
`167`	`167`	`reinterpret_cast<HRESULT>(response.Payload.data()) = hr;`
`@@ -175,16 +175,16 @@ bool CommandServer::IsControlCommand(const IpcMessage& message)`
`175`	`175`	`case static_cast<int>(CommandSet::Profiler):`
`176`	`176`	`switch (message.Command)`
`177`	`177`	`{`
`178`		`- case static_cast<int>(ProfilerCommand::Start):`
`179`		`- case static_cast<int>(ProfilerCommand::Stop):`
	`178`	`+ case static_cast<int>(ProfilerCommand::StartAllFeatures):`
	`179`	`+ case static_cast<int>(ProfilerCommand::StopAllFeatures):`
`180`	`180`	`return true;`
`181`	`181`	`default:`
`182`	`182`	`return false;`
`183`	`183`	`}`
`184`	`184`	`case static_cast<int>(CommandSet::StartupHook):`
`185`	`185`	`switch (message.Command) {`
`186`		`- case static_cast<int>(StartupHookCommand::Start):`
`187`		`- case static_cast<int>(StartupHookCommand::Stop):`
	`186`	`+ case static_cast<int>(StartupHookCommand::StartAllFeatures):`
	`187`	`+ case static_cast<int>(StartupHookCommand::StopAllFeatures):`
`188`	`188`	`return true;`
`189`	`189`	`default:`
`190`	`190`	`return false;`
`@@ -250,9 +250,9 @@ void CommandServer::ProcessingThread(BlockingQueue<CallbackInfo>& queue)`
`250`	`250`	`_logger->Log(LogLevel::Warning, _LS("IpcMessage callback failed: 0x%08x"), hr);`
`251`	`251`	`}`
`252`	`252`
`253`		`- if (info.Promise)`
	`253`	`+ if (info.CompletionPromise)`
`254`	`254`	`{`
`255`		`- info.Promise->set_value(hr);`
	`255`	`+ info.CompletionPromise->set_value(hr);`
`256`	`256`	`}`
`257`	`257`	`}`
`258`	`258`	`}`