Remove Serilog and adjust factor for HostLogger injection

Author: JustinGrote

.pipelines/PowerShellEditorServices-Official.yml

@@ -132,7 +132,6 @@ extends:
                 **/Nerdbank.Streams.dll;
                 **/Newtonsoft.Json.dll;
                 **/OmniSharp.Extensions*.dll;
-                **/Serilog*.dll;
                 **/System.Reactive.dll;
           - task: ArchiveFiles@2
             displayName: Zip signed artifacts

Directory.Packages.props

@@ -12,10 +12,6 @@
     <PackageVersion Include="OmniSharp.Extensions.LanguageClient" Version="0.19.9" />
     <PackageVersion Include="OmniSharp.Extensions.LanguageServer" Version="0.19.9" />
     <PackageVersion Include="PowerShellStandard.Library" Version="5.1.1" />
-    <PackageVersion Include="Serilog" Version="4.0.0" />
-    <PackageVersion Include="Serilog.Extensions.Logging" Version="8.0.0" />
-    <PackageVersion Include="Serilog.Sinks.Async" Version="2.0.0" />
-    <PackageVersion Include="Serilog.Sinks.File" Version="6.0.0" />
     <PackageVersion Include="System.IO.Pipes.AccessControl" Version="5.0.0" />
     <PackageVersion Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" />
     <PackageVersion Include="System.Security.Principal" Version="4.3.0" />

NOTICE.txt

@@ -219,7 +219,6 @@ protected override void BeginProcessing()
         protected override void EndProcessing()
         {
             _logger.Log(PsesLogLevel.Diagnostic, "Beginning EndProcessing block");
-
             try
             {
                 // First try to remove PSReadLine to decomplicate startup

src/PowerShellEditorServices.Hosting/Commands/StartEditorServicesCommand.cs

@@ -15,9 +15,8 @@ namespace Microsoft.PowerShell.EditorServices.Hosting
     /// User-facing log level for editor services configuration.
     /// </summary>
     /// <remarks>
-    /// The underlying values of this enum attempt to align to both
-    /// <see cref="Microsoft.Extensions.Logging.LogLevel" /> and
-    /// <see cref="Serilog.Events.LogEventLevel" />.
+    /// The underlying values of this enum attempt to align to
+    /// <see cref="Microsoft.Extensions.Logging.LogLevel" />
     /// </remarks>
     public enum PsesLogLevel
     {

src/PowerShellEditorServices.Hosting/Configuration/HostLogger.cs

@@ -42,7 +42,7 @@ public EditorServicesRunner(
             _config = config;
             _sessionFileWriter = sessionFileWriter;
             // NOTE: This factory helps to isolate `Microsoft.Extensions.Logging/DependencyInjection`.
-            _serverFactory = EditorServicesServerFactory.Create(_config.LogPath, (int)_config.LogLevel, logger);
+            _serverFactory = new(logger);
             _alreadySubscribedDebug = false;
             _loggersToUnsubscribe = loggersToUnsubscribe;
         }

src/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.cs

@@ -2,81 +2,31 @@
 // Licensed under the MIT License.
 
 using System;
-using System.Diagnostics;
 using System.IO;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
-using Microsoft.PowerShell.EditorServices.Logging;
 using Microsoft.PowerShell.EditorServices.Server;
-using Serilog;
-using Serilog.Events;
 using OmniSharp.Extensions.LanguageServer.Protocol.Server;
 using Microsoft.PowerShell.EditorServices.Services.Extension;
+using OmniSharp.Extensions.LanguageServer.Server;
 
-#if DEBUG
-using Serilog.Debugging;
-#endif
+// The HostLogger type isn't directly referenced from this assembly, however it uses a common IObservable interface and this alias helps make it more clear the purpose. We can use Microsoft.Extensions.Logging from this point because the ALC should be loaded, but we need to only expose the IObservable to the Hosting assembly so it doesn't try to load MEL before the ALC is ready.
+using HostLogger = System.IObservable<(int logLevel, string message)>;
 
 namespace Microsoft.PowerShell.EditorServices.Hosting
 {
     /// <summary>
-    /// Factory class for hiding dependencies of Editor Services.
+    /// Factory for creating the LSP server and debug server instances.
     /// </summary>
-    /// <remarks>
-    /// Dependency injection and logging are wrapped by factory methods on this class so that the
-    /// host assembly can construct the LSP and debug servers without directly depending on <see
-    /// cref="Microsoft.Extensions.Logging"/> and <see
-    /// cref="Microsoft.Extensions.DependencyInjection"/>.
-    /// </remarks>
     internal sealed class EditorServicesServerFactory : IDisposable
     {
+        private readonly HostLogger _hostLogger;
+
         /// <summary>
-        /// Create a new Editor Services factory. This method will instantiate logging.
+        /// Creates a loggerfactory for this instance
         /// </summary>
-        /// <remarks>
-        /// <para>
-        /// This can only be called once because it sets global state (the logger) and that call is
-        /// in <see cref="Hosting.EditorServicesRunner" />.
-        /// </para>
-        /// <para>
-        /// TODO: Why is this a static function wrapping a constructor instead of just a
-        /// constructor? In the end it returns an instance (albeit a "singleton").
-        /// </para>
-        /// </remarks>
-        /// <param name="logDirectoryPath">The path of the log file to use.</param>
-        /// <param name="minimumLogLevel">The minimum log level to use.</param>
-        /// <param name="hostLogger">The host logger?</param>
-        public static EditorServicesServerFactory Create(string logDirectoryPath, int minimumLogLevel, IObservable<(int logLevel, string message)> hostLogger)
-        {
-            // NOTE: Ignore the suggestion to use Environment.ProcessId as it doesn't work for
-            // .NET 4.6.2 (for Windows PowerShell), and this won't be caught in CI.
-            int currentPID = Process.GetCurrentProcess().Id;
-            string logPath = Path.Combine(logDirectoryPath, $"PowerShellEditorServices-{currentPID}.log");
-            Log.Logger = new LoggerConfiguration()
-                .Enrich.FromLogContext()
-                .WriteTo.Async(config => config.File(logPath))
-                .MinimumLevel.Is((LogEventLevel)minimumLogLevel)
-                .CreateLogger();
-
-#if DEBUG
-            SelfLog.Enable(msg => Debug.WriteLine(msg));
-#endif
-
-            LoggerFactory loggerFactory = new();
-            loggerFactory.AddSerilog();
-
-            // Hook up logging from the host so that its recorded in the log file
-            hostLogger.Subscribe(new HostLoggerAdapter(loggerFactory));
-
-            return new EditorServicesServerFactory(loggerFactory);
-        }
-
-        // TODO: Can we somehow refactor this member so the language and debug servers can be
-        // instantiated using their constructors instead of tying them to this factory with `Create`
-        // methods?
-        private readonly ILoggerFactory _loggerFactory;
-
-        private EditorServicesServerFactory(ILoggerFactory loggerFactory) => _loggerFactory = loggerFactory;
+        /// <param name="hostLogger">The hostLogger that will be provided to the language services for logging handoff</param>
+        internal EditorServicesServerFactory(HostLogger hostLogger) => _hostLogger = hostLogger;
 
         /// <summary>
         /// Create the LSP server.
@@ -92,7 +42,7 @@ public static EditorServicesServerFactory Create(string logDirectoryPath, int mi
         public PsesLanguageServer CreateLanguageServer(
             Stream inputStream,
             Stream outputStream,
-            HostStartupInfo hostStartupInfo) => new(_loggerFactory, inputStream, outputStream, hostStartupInfo);
+            HostStartupInfo hostStartupInfo) => new(_hostLogger, inputStream, outputStream, hostStartupInfo);
 
         /// <summary>
         /// Create the debug server given a language server instance.
@@ -110,7 +60,7 @@ public PsesDebugServer CreateDebugServerWithLanguageServer(
             PsesLanguageServer languageServer)
         {
             return new PsesDebugServer(
-                _loggerFactory,
+                _hostLogger,
                 inputStream,
                 outputStream,
                 languageServer.LanguageServer.Services);
@@ -132,7 +82,7 @@ public PsesDebugServer RecreateDebugServer(
             PsesDebugServer debugServer)
         {
             return new PsesDebugServer(
-                _loggerFactory,
+                _hostLogger,
                 inputStream,
                 outputStream,
                 debugServer.ServiceProvider);
@@ -153,7 +103,7 @@ public PsesDebugServer CreateDebugServerForTempSession(
             ServiceProvider serviceProvider = new ServiceCollection()
                 .AddLogging(builder => builder
                     .ClearProviders()
-                    .AddSerilog()
+                    .AddLanguageProtocolLogging()
                     .SetMinimumLevel(LogLevel.Trace)) // TODO: Why randomly set to trace?
                 .AddSingleton<ILanguageServerFacade>(_ => null)
                 // TODO: Why add these for a debug server?!
@@ -171,25 +121,14 @@ public PsesDebugServer CreateDebugServerForTempSession(
             serviceProvider.GetService<ExtensionService>();
 
             return new PsesDebugServer(
-                _loggerFactory,
+                _hostLogger,
                 inputStream,
                 outputStream,
                 serviceProvider,
                 isTemp: true);
         }
 
-        /// <summary>
-        /// TODO: This class probably should not be <see cref="IDisposable"/> as the primary
-        /// intention of that interface is to provide cleanup of unmanaged resources, which the
-        /// logger certainly is not. Nor is this class used with a <see langword="using"/>. Instead,
-        /// this class should call <see cref="Log.CloseAndFlush()"/> in a finalizer. This
-        /// could potentially even be done with <see
-        /// cref="SerilogLoggerFactoryExtensions.AddSerilog"</> by passing <c>dispose=true</c>.
-        /// </summary>
-        public void Dispose()
-        {
-            Log.CloseAndFlush();
-            _loggerFactory.Dispose();
-        }
+        // TODO: Clean up host logger? Shouldn't matter since we start a new process after shutdown.
+        public void Dispose() { }
     }
 }

src/PowerShellEditorServices/Hosting/EditorServicesServerFactory.cs

@@ -107,9 +107,8 @@ public sealed class HostStartupInfo
         /// The minimum log level of log events to be logged.
         /// </summary>
         /// <remarks>
-        /// This is cast to all of <see cref="Hosting.PsesLogLevel"/>, <see
-        /// cref="Microsoft.Extensions.Logging.LogLevel"/>, and <see
-        /// cref="Serilog.Events.LogEventLevel"/>, hence it is an <c>int</c>.
+        /// This primitive maps to <see cref="Hosting.PsesLogLevel"/> and <see
+        /// cref="Microsoft.Extensions.Logging.LogLevel"/>
         /// </remarks>
         public int LogLevel { get; }
 

src/PowerShellEditorServices/Hosting/HostStartupInfo.cs

@@ -9,23 +9,16 @@ namespace Microsoft.PowerShell.EditorServices.Logging
     /// <summary>
     /// Adapter class to allow logging events sent by the host to be recorded by PSES' logging infrastructure.
     /// </summary>
-    internal class HostLoggerAdapter : IObserver<(int logLevel, string message)>
+    internal class HostLoggerAdapter(ILogger logger) : IObserver<(int logLevel, string message)>
     {
-        private readonly ILogger _logger;
+        public void OnError(Exception error) => logger.LogError(error, "Error in host logger");
 
-        /// <summary>
-        /// Create a new host logger adapter.
-        /// </summary>
-        /// <param name="loggerFactory">Factory to create logger instances with.</param>
-        public HostLoggerAdapter(ILoggerFactory loggerFactory) => _logger = loggerFactory.CreateLogger("HostLogs");
+        public void OnNext((int logLevel, string message) value) => logger.Log((LogLevel)value.logLevel, value.message);
 
         public void OnCompleted()
         {
             // Nothing to do; we simply don't send more log messages
         }
 
-        public void OnError(Exception error) => _logger.LogError(error, "Error in host logger");
-
-        public void OnNext((int logLevel, string message) value) => _logger.Log((LogLevel)value.logLevel, value.message);
     }
 }

src/PowerShellEditorServices/Logging/HostLoggerAdapter.cs

@@ -36,10 +36,6 @@
     <PackageReference Include="Microsoft.Extensions.FileSystemGlobbing" />
     <PackageReference Include="Microsoft.Extensions.Logging" />
     <PackageReference Include="PowerShellStandard.Library" />
-    <PackageReference Include="Serilog" />
-    <PackageReference Include="Serilog.Extensions.Logging" />
-    <PackageReference Include="Serilog.Sinks.Async" />
-    <PackageReference Include="Serilog.Sinks.File" />
     <PackageReference Include="System.IO.Pipes.AccessControl" />
     <PackageReference Include="System.Security.Principal" />
     <PackageReference Include="System.Security.Principal.Windows" />

src/PowerShellEditorServices/PowerShellEditorServices.csproj

@@ -5,13 +5,15 @@
 using System.IO;
 using System.Threading.Tasks;
 using Microsoft.Extensions.DependencyInjection;
-using Microsoft.Extensions.Logging;
 using Microsoft.PowerShell.EditorServices.Handlers;
 using Microsoft.PowerShell.EditorServices.Services;
 using Microsoft.PowerShell.EditorServices.Services.PowerShell.Host;
 using OmniSharp.Extensions.DebugAdapter.Server;
 using OmniSharp.Extensions.LanguageServer.Server;
 
+// See EditorServicesServerFactory.cs for the explanation of this alias.
+using HostLogger = System.IObservable<(int logLevel, string message)>;
+
 namespace Microsoft.PowerShell.EditorServices.Server
 {
     /// <summary>
@@ -26,16 +28,17 @@ internal class PsesDebugServer : IDisposable
         private PsesInternalHost _psesHost;
         private bool _startedPses;
         private readonly bool _isTemp;
-        protected readonly ILoggerFactory _loggerFactory;
+        // FIXME: This was never actually used in the debug server. Since we never have a debug server without an LSP, we could probably remove this and either reuse the MEL from the LSP, or create a new one here. It is probably best to only use this for exceptions that we can't reasonably send via the DAP protocol, which should only be anything before the initialize request.
+        protected readonly HostLogger _hostLogger;
 
         public PsesDebugServer(
-            ILoggerFactory factory,
+            HostLogger hostLogger,
             Stream inputStream,
             Stream outputStream,
             IServiceProvider serviceProvider,
             bool isTemp = false)
         {
-            _loggerFactory = factory;
+            _hostLogger = hostLogger;
             _inputStream = inputStream;
             _outputStream = outputStream;
             ServiceProvider = serviceProvider;
@@ -130,7 +133,6 @@ public void Dispose()
             _debugAdapterServer?.Dispose();
             _inputStream.Dispose();
             _outputStream.Dispose();
-            _loggerFactory.Dispose();
             _serverStopped.SetResult(true);
             // TODO: If the debugger has stopped, should we clear the breakpoints?
         }