Address PR comments

include/onnxruntime/core/common/logging/isink.h

@@ -6,17 +6,15 @@
 #include <string>
 
 #include "core/common/logging/logging.h"
+#include "core/common/logging/sink_types.h"
 
 namespace onnxruntime {
 namespace logging {
 class ISink {
  public:
-  ISink() = default;
+  explicit ISink(SinkType type = SinkType::BaseSink) : type_(type) {}
 
-  enum SinkType { BaseSink,
-                  CompositeSink,
-                  EtwSink };
-  virtual SinkType GetType() const { return BaseSink; }
+  SinkType GetType() const { return type_; }
 
   /**
      Sends the message to the sink.
@@ -37,6 +35,8 @@ class ISink {
   virtual ~ISink() = default;
 
  private:
+  SinkType type_;
+
   // Make Code Analysis happy by disabling all for now. Enable as needed.
   ORT_DISALLOW_COPY_ASSIGNMENT_AND_MOVE(ISink);
 

include/onnxruntime/core/common/logging/logging.h

@@ -16,6 +16,7 @@
 #include "core/common/logging/capture.h"
 #include "core/common/logging/macros.h"
 #include "core/common/logging/severity.h"
+#include "core/common/logging/sink_types.h"
 #include "core/platform/ort_mutex.h"
 #include "date/date.h"
 
@@ -171,18 +172,16 @@ class LoggingManager final {
   */
   static LoggingManager* GetDefaultInstance();
 
-#ifdef _WIN32
   /**
-     Removes the ETW Sink if one is present
+     Removes a Sink if one is present
   */
-  void RemoveEtwSink();
+  void RemoveSink(SinkType sinkType);
 
   /**
-     Adds an ETW Sink to the current sink creating a CompositeSink if necessary
-     @param etwSeverity The severity level for the ETW Sink
+     Adds a Sink to the current sink creating a CompositeSink if necessary
+     @param severity The severity level for the new Sink
   */
-  void AddEtwSink(logging::Severity etwSeverity);
-#endif
+  void AddSink(SinkType sinkType, std::function<std::unique_ptr<ISink>()> sinkFactory, logging::Severity severity);
 
   /**
      Change the minimum severity level for log messages to be output by the default logger.

include/onnxruntime/core/common/logging/sink_types.h

@@ -0,0 +1,11 @@
+#pragma once
+
+namespace onnxruntime {
+namespace logging {
+enum class SinkType {
+  BaseSink,
+  CompositeSink,
+  EtwSink
+};
+}  // namespace logging
+}  // namespace onnxruntime

onnxruntime/core/common/logging/logging.cc

@@ -281,70 +281,62 @@ Severity OverrideLevelWithEtw(Severity originalSeverity) {
   return originalSeverity;
 }
 
-#ifdef _WIN32
-void LoggingManager::AddEtwSink(logging::Severity etwSeverity) {
+void LoggingManager::AddSink(SinkType sinkType, std::function<std::unique_ptr<ISink>()> sinkFactory, logging::Severity severity) {
   std::lock_guard<OrtMutex> guard(sink_mutex_);
 
-  // Check if the EtwRegistrationManager is enabled
-  auto& manager = EtwRegistrationManager::Instance();
-  if (!manager.IsEnabled()) {
-    return;  // ETW not enabled, no operation needed
-  }
-
-  if (sink_->GetType() != ISink::CompositeSink) {
+  if (sink_->GetType() != SinkType::CompositeSink) {
     // Current sink is not a composite, create a new composite sink and add the current sink to it
     auto new_composite = std::make_unique<CompositeSink>();
     new_composite->AddSink(std::move(sink_), default_min_severity_);  // Move the current sink into the new composite
     sink_ = std::move(new_composite);                                 // Now sink_ is pointing to the new composite
   }
 
-  // Adjust the default minimum severity level to accommodate ETW logging needs
-  default_min_severity_ = std::min(default_min_severity_, etwSeverity);
+  // Adjust the default minimum severity level to accommodate new sink needs
+  default_min_severity_ = std::min(default_min_severity_, severity);
   if (s_default_logger_ != nullptr) {
     s_default_logger_->SetSeverity(default_min_severity_);
   }
 
   CompositeSink* current_composite = static_cast<CompositeSink*>(sink_.get());
-  // Check if an EtwSink already exists in the current composite
   const auto& sinks = current_composite->GetSinks();
-  if (std::any_of(sinks.begin(), sinks.end(), [](const auto& pair) {
-        return pair.first->GetType() == ISink::EtwSink;
+
+  // Check if a sink of this type already exists
+  if (std::any_of(sinks.begin(), sinks.end(), [sinkType](const auto& pair) {
+        return pair.first->GetType() == sinkType;
       })) {
-    return;  // EtwSink already exists, do not add another
+    return;  // Sink of this type already exists, do not add another
   }
 
-  // Add a new EtwSink
-  current_composite->AddSink(std::make_unique<EtwSink>(), etwSeverity);
+  current_composite->AddSink(sinkFactory(), severity);
 }
 
-void LoggingManager::RemoveEtwSink() {
+void LoggingManager::RemoveSink(SinkType sinkType) {
   std::lock_guard<OrtMutex> guard(sink_mutex_);
 
-  if (sink_->GetType() == ISink::CompositeSink) {
+  if (sink_->GetType() == SinkType::CompositeSink) {
     auto composite_sink = static_cast<CompositeSink*>(sink_.get());
     const auto& sinks_with_severity = composite_sink->GetSinks();
     std::unique_ptr<ISink> remaining_sink;
 
     Severity newSeverity = Severity::kFATAL;
 
     for (const auto& sink_pair : sinks_with_severity) {
-      if (sink_pair.first->GetType() != ISink::EtwSink) {
+      if (sink_pair.first->GetType() != sinkType) {
         if (remaining_sink) {
-          // If more than one non-EtwSink is found, we leave the CompositeSink intact
+          // If more than one non sinkType is found, we leave the CompositeSink intact
           return;
         }
         newSeverity = std::min(newSeverity, sink_pair.second);
         remaining_sink = std::move(const_cast<std::unique_ptr<ISink>&>(sink_pair.first));
       }
     }
 
-    // If only one non-EtwSink remains, replace the CompositeSink with this sink
+    // If only one non sinkType remains, replace the CompositeSink with this sink
     if (remaining_sink) {
       sink_ = std::move(remaining_sink);
 
     } else {
-      // Handle the case where all sinks were EtwSinks
-      // sink_ = std::make_unique<NullSink>(); // Assuming NullSink is a basic ISink that does nothing
+      // Handle the case where all sinks were sinkType
     }
 
     default_min_severity_ = newSeverity;
@@ -353,7 +345,6 @@ void LoggingManager::RemoveEtwSink() {
     }
   }
 }
-#endif
 
 }  // namespace logging
 }  // namespace onnxruntime

onnxruntime/core/common/logging/sinks/composite_sink.h

@@ -23,9 +23,7 @@ class CompositeSink : public ISink {
   /// Initializes a new instance of the <see cref="CompositeSink"/> class.
   /// Use AddSink to add sinks.
   /// </summary>
-  CompositeSink() {}
-
-  SinkType GetType() const override { return ISink::CompositeSink; }
+  CompositeSink() : ISink(SinkType::CompositeSink) {}
 
   /// <summary>
   /// Adds a sink. Takes ownership of the sink (so pass unique_ptr by value).

onnxruntime/core/platform/windows/logging/etw_sink.h

@@ -31,11 +31,9 @@ namespace logging {
 
 class EtwSink : public ISink {
  public:
-  EtwSink() = default;
+  EtwSink() : ISink(SinkType::EtwSink) {}
   ~EtwSink() = default;
 
-  SinkType GetType() const override { return ISink::EtwSink; }
-
   constexpr static const char* kEventName = "ONNXRuntimeLogEvent";
 
  private:

onnxruntime/core/session/inference_session.cc

@@ -12,6 +12,7 @@
 #include <queue>
 
 #include "core/common/denormal.h"
+#include "core/common/logging/isink.h"
 #include "core/common/logging/logging.h"
 #include "core/common/parse_string.h"
 #include "core/common/path_string.h"
@@ -52,6 +53,7 @@
 #include "core/platform/tracing.h"
 #include <Windows.h>
 #include "core/platform/windows/telemetry.h"
+#include "core/platform/windows/logging/etw_sink.h"
 #endif
 #include "core/providers/cpu/controlflow/utils.h"
 #include "core/providers/cpu/cpu_execution_provider.h"
@@ -420,13 +422,19 @@ void InferenceSession::ConstructorCommon(const SessionOptions& session_options,
           if ((MatchAnyKeyword & static_cast<ULONGLONG>(onnxruntime::logging::ORTTraceLoggingKeyword::Logs)) != 0 &&
               IsEnabled == EVENT_CONTROL_CODE_ENABLE_PROVIDER) {
             LOGS(*session_logger_, VERBOSE) << "Adding ETW Sink to logger with severity level: " << (ULONG)ortETWSeverity;
-            logging_manager_->AddEtwSink(ortETWSeverity);
-            onnxruntime::logging::LoggingManager::GetDefaultInstance()->AddEtwSink(ortETWSeverity);
+            logging_manager_->AddSink(
+                onnxruntime::logging::SinkType::EtwSink,
+                []() -> std::unique_ptr<onnxruntime::logging::ISink> { return std::make_unique<onnxruntime::logging::EtwSink>(); },
+                ortETWSeverity);
+            onnxruntime::logging::LoggingManager::GetDefaultInstance()->AddSink(
+                onnxruntime::logging::SinkType::EtwSink,
+                []() -> std::unique_ptr<onnxruntime::logging::ISink> { return std::make_unique<onnxruntime::logging::EtwSink>(); },
+                ortETWSeverity);
             LOGS(*session_logger_, INFO) << "Done Adding ETW Sink to logger with severity level: " << (ULONG)ortETWSeverity;
           }
           if (IsEnabled == EVENT_CONTROL_CODE_DISABLE_PROVIDER) {
             LOGS(*session_logger_, INFO) << "Removing ETW Sink from logger";
-            logging_manager_->RemoveEtwSink();
+            logging_manager_->RemoveSink(onnxruntime::logging::SinkType::EtwSink);
             LOGS(*session_logger_, VERBOSE) << "Done Removing ETW Sink from logger";
           }
         }