Remove Serilog and adjust factor for HostLogger injection

.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" />

src/PowerShellEditorServices.Hosting/Internal/EditorServicesRunner.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/EditorServicesServerFactory.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/PowerShellEditorServices.csproj

@@ -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/Server/PsesDebugServer.cs

@@ -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?
         }

src/PowerShellEditorServices/Server/PsesLanguageServer.cs

@@ -15,7 +15,9 @@
 using OmniSharp.Extensions.JsonRpc;
 using OmniSharp.Extensions.LanguageServer.Protocol.Server;
 using OmniSharp.Extensions.LanguageServer.Server;
-using Serilog;
+
+// See EditorServicesServerFactory.cs for the explanation of this alias.
+using HostLogger = System.IObservable<(int logLevel, string message)>;
 
 namespace Microsoft.PowerShell.EditorServices.Server
 {
@@ -24,7 +26,7 @@ namespace Microsoft.PowerShell.EditorServices.Server
     /// </summary>
     internal class PsesLanguageServer
     {
-        internal ILoggerFactory LoggerFactory { get; }
+        internal HostLogger LoggerFactory { get; }
         internal ILanguageServer LanguageServer { get; private set; }
         private readonly LogLevel _minimumLogLevel;
         private readonly Stream _inputStream;
@@ -47,7 +49,7 @@ internal class PsesLanguageServer
         /// <param name="hostStartupInfo">Host configuration to instantiate the server and services
         /// with.</param>
         public PsesLanguageServer(
-            ILoggerFactory factory,
+            HostLogger factory,
             Stream inputStream,
             Stream outputStream,
             HostStartupInfo hostStartupInfo)
@@ -82,8 +84,9 @@ public async Task StartAsync()
                         serviceCollection.AddPsesLanguageServices(_hostDetails);
                     })
                     .ConfigureLogging(builder => builder
-                        .AddSerilog(Log.Logger) // TODO: Set dispose to true?
+                        .ClearProviders()
                         .AddLanguageProtocolLogging()
+                        // TODO: AddHostLogger which registers the host logger provider above as a LoggingProvider (MEL version of a "sink")
                         .SetMinimumLevel(_minimumLogLevel))
                     // TODO: Consider replacing all WithHandler with AddSingleton
                     .WithHandler<PsesWorkspaceSymbolsHandler>()