Switch to native .NET logging APIs

aspnet · Jul 18, 2016 · f4efcac · f4efcac
1 parent 27ffa72
commit f4efcac
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 54 deletions.
diff --git a/src/Microsoft.AspNetCore.NodeServices/Configuration/Configuration.cs b/src/Microsoft.AspNetCore.NodeServices/Configuration/Configuration.cs
@@ -2,11 +2,15 @@
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.NodeServices.HostingModels;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Console;
 
 namespace Microsoft.AspNetCore.NodeServices
 {
     public static class Configuration
     {
+        const string LogCategoryName = "Microsoft.AspNetCore.NodeServices";
+
         public static void AddNodeServices(this IServiceCollection serviceCollection)
             => AddNodeServices(serviceCollection, new NodeServicesOptions());
 
@@ -15,13 +19,24 @@ public static void AddNodeServices(this IServiceCollection serviceCollection, No
             serviceCollection.AddSingleton(typeof(INodeServices), serviceProvider =>
             {
                 // Since this instance is being created through DI, we can access the IHostingEnvironment
-                // to populate options.ProjectPath if it wasn't explicitly specified.
-                var hostEnv = serviceProvider.GetRequiredService<IHostingEnvironment>();
+                // to populate options.ProjectPath if it wasn't explicitly specified.                
                 if (string.IsNullOrEmpty(options.ProjectPath))
                 {
+                    var hostEnv = serviceProvider.GetRequiredService<IHostingEnvironment>();
                     options.ProjectPath = hostEnv.ContentRootPath;
                 }
 
+                // Likewise, if no logger was specified explicitly, we should use the one from DI.
+                // If it doesn't provide one, CreateNodeInstance will set up a default.
+                if (options.NodeInstanceOutputLogger == null)
+                {
+                    var loggerFactory = serviceProvider.GetService<ILoggerFactory>();
+                    if (loggerFactory != null)
+                    {
+                        options.NodeInstanceOutputLogger = loggerFactory.CreateLogger(LogCategoryName); 
+                    }
+                }
+
                 return new NodeServicesImpl(options, () => CreateNodeInstance(options));
             });
         }
@@ -33,6 +48,13 @@ public static INodeServices CreateNodeServices(NodeServicesOptions options)
 
         private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
         {
+            // If you've specified no logger, fall back on a default console logger
+            var logger = options.NodeInstanceOutputLogger;
+            if (logger == null)
+            {
+                logger = new ConsoleLogger(LogCategoryName, null, false);
+            }
+
             if (options.NodeInstanceFactory != null)
             {
                 // If you've explicitly supplied an INodeInstance factory, we'll use that. This is useful for
@@ -46,10 +68,10 @@ private static INodeInstance CreateNodeInstance(NodeServicesOptions options)
                 switch (options.HostingModel)
                 {
                     case NodeHostingModel.Http:
-                        return new HttpNodeInstance(options.ProjectPath, options.WatchFileExtensions, /* port */ 0, options.NodeInstanceOutputLogger);
+                        return new HttpNodeInstance(options.ProjectPath, options.WatchFileExtensions, logger, /* port */ 0);
                     case NodeHostingModel.Socket:
                         var pipeName = "pni-" + Guid.NewGuid().ToString("D"); // Arbitrary non-clashing string
-                        return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions, pipeName, options.NodeInstanceOutputLogger);
+                        return new SocketNodeInstance(options.ProjectPath, options.WatchFileExtensions, pipeName, logger);
                     default:
                         throw new ArgumentException("Unknown hosting model: " + options.HostingModel);
                 }

diff --git a/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesOptions.cs b/src/Microsoft.AspNetCore.NodeServices/Configuration/NodeServicesOptions.cs
@@ -1,6 +1,6 @@
 using System;
 using Microsoft.AspNetCore.NodeServices.HostingModels;
-using Microsoft.AspNetCore.NodeServices.Util;
+using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.NodeServices
 {
@@ -20,6 +20,6 @@ public NodeServicesOptions()
         public Func<INodeInstance> NodeInstanceFactory { get; set; }
         public string ProjectPath { get; set; }
         public string[] WatchFileExtensions { get; set; }
-        public INodeInstanceOutputLogger NodeInstanceOutputLogger { get; set; }
+        public ILogger NodeInstanceOutputLogger { get; set; }
     }
 }
diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/HttpNodeInstance.cs
@@ -4,7 +4,7 @@
 using System.Text;
 using System.Text.RegularExpressions;
 using System.Threading.Tasks;
-using Microsoft.AspNetCore.NodeServices.Util;
+using Microsoft.Extensions.Logging;
 using Newtonsoft.Json;
 using Newtonsoft.Json.Serialization;
 
@@ -33,7 +33,7 @@ internal class HttpNodeInstance : OutOfProcessNodeInstance
         private bool _disposed;
         private int _portNumber;
 
-        public HttpNodeInstance(string projectPath, string[] watchFileExtensions, int port = 0, INodeInstanceOutputLogger nodeInstanceOutputLogger = null)
+        public HttpNodeInstance(string projectPath, string[] watchFileExtensions, ILogger nodeInstanceOutputLogger, int port = 0)
             : base(
                 EmbeddedResourceReader.Read(
                     typeof(HttpNodeInstance),

diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/OutOfProcessNodeInstance.cs
@@ -3,7 +3,7 @@
 using System.IO;
 using System.Linq;
 using System.Threading.Tasks;
-using Microsoft.AspNetCore.NodeServices.Util;
+using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.NodeServices.HostingModels
 {
@@ -19,6 +19,7 @@ namespace Microsoft.AspNetCore.NodeServices.HostingModels
     /// <seealso cref="Microsoft.AspNetCore.NodeServices.HostingModels.INodeInstance" />
     public abstract class OutOfProcessNodeInstance : INodeInstance
     {
+        protected readonly ILogger OutputLogger;
         private const string ConnectionEstablishedMessage = "[Microsoft.AspNetCore.NodeServices:Listening]";
         private readonly TaskCompletionSource<object> _connectionIsReadySource = new TaskCompletionSource<object>();
         private bool _disposed;
@@ -27,18 +28,20 @@ public abstract class OutOfProcessNodeInstance : INodeInstance
         private readonly Process _nodeProcess;
         private bool _nodeProcessNeedsRestart;
         private readonly string[] _watchFileExtensions;
-        private INodeInstanceOutputLogger _nodeInstanceOutputLogger;
 
         public OutOfProcessNodeInstance(
             string entryPointScript,
             string projectPath,
             string[] watchFileExtensions,
             string commandLineArguments,
-            INodeInstanceOutputLogger nodeOutputLogger)
+            ILogger nodeOutputLogger)
         {
-            _nodeInstanceOutputLogger = nodeOutputLogger;
-            if(_nodeInstanceOutputLogger == null)
-                _nodeInstanceOutputLogger = new ConsoleNodeInstanceOutputLogger();
+            if (nodeOutputLogger == null)
+            {
+                throw new ArgumentNullException(nameof(nodeOutputLogger));
+            }
+
+            OutputLogger = nodeOutputLogger;
             _entryPointScript = new StringAsTempFile(entryPointScript);
 
             var startInfo = PrepareNodeProcessStartInfo(_entryPointScript.FileName, projectPath, commandLineArguments);
@@ -112,12 +115,12 @@ protected virtual ProcessStartInfo PrepareNodeProcessStartInfo(
 
         protected virtual void OnOutputDataReceived(string outputData)
         {
-            _nodeInstanceOutputLogger.LogOutputData(outputData);
+            OutputLogger.LogInformation(outputData);
         }
 
         protected virtual void OnErrorDataReceived(string errorData)
         {
-            _nodeInstanceOutputLogger.LogErrorData(errorData);
+            OutputLogger.LogError(errorData);
         }
 
         protected virtual void Dispose(bool disposing)
@@ -255,8 +258,7 @@ private bool IsFilenameBeingWatched(string fullPath)
 
         private void RestartDueToFileChange(string fullPath)
         {
-            // TODO: Use proper logger
-            Console.WriteLine($"Node will restart because file changed: {fullPath}");
+            OutputLogger.LogInformation($"Node will restart because file changed: {fullPath}");
 
             _nodeProcessNeedsRestart = true;
 

diff --git a/src/Microsoft.AspNetCore.NodeServices/HostingModels/SocketNodeInstance.cs b/src/Microsoft.AspNetCore.NodeServices/HostingModels/SocketNodeInstance.cs
@@ -5,7 +5,7 @@
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.NodeServices.HostingModels.PhysicalConnections;
 using Microsoft.AspNetCore.NodeServices.HostingModels.VirtualConnections;
-using Microsoft.AspNetCore.NodeServices.Util;
+using Microsoft.Extensions.Logging;
 using Newtonsoft.Json;
 using Newtonsoft.Json.Serialization;
 
@@ -37,7 +37,7 @@ internal class SocketNodeInstance : OutOfProcessNodeInstance
         private string _socketAddress;
         private VirtualConnectionClient _virtualConnectionClient;
 
-        public SocketNodeInstance(string projectPath, string[] watchFileExtensions, string socketAddress, INodeInstanceOutputLogger nodeInstanceOutputLogger = null) : base(
+        public SocketNodeInstance(string projectPath, string[] watchFileExtensions, string socketAddress, ILogger nodeInstanceOutputLogger) : base(
                 EmbeddedResourceReader.Read(
                     typeof(SocketNodeInstance),
                     "/Content/Node/entrypoint-socket.js"),
@@ -125,9 +125,7 @@ private async Task EnsureVirtualConnectionClientCreated()
                         // failure, this Node instance is no longer usable and should be discarded.
                         _connectionHasFailed = true;
 
-                        // TODO: Log the exception properly. Need to change the chain of calls up to this point to supply
-                        // an ILogger or IServiceProvider etc.
-                        Console.WriteLine(ex.Message);
+                        OutputLogger.LogError(0, ex, ex.Message);
                     };
                 }
             }

diff --git a/src/Microsoft.AspNetCore.NodeServices/Util/ConsoleNodeInstanceOutputLogger.cs b/src/Microsoft.AspNetCore.NodeServices/Util/ConsoleNodeInstanceOutputLogger.cs
diff --git a/src/Microsoft.AspNetCore.NodeServices/Util/INodeInstanceOutputLogger.cs b/src/Microsoft.AspNetCore.NodeServices/Util/INodeInstanceOutputLogger.cs
diff --git a/src/Microsoft.AspNetCore.NodeServices/project.json b/src/Microsoft.AspNetCore.NodeServices/project.json
@@ -9,6 +9,7 @@
     "Microsoft.AspNetCore.Hosting.Abstractions": "1.0.0",
     "Microsoft.Extensions.Configuration.Json": "1.0.0",
     "Microsoft.Extensions.DependencyInjection.Abstractions": "1.0.0",
+    "Microsoft.Extensions.Logging.Console": "1.0.0",
     "Microsoft.Extensions.PlatformAbstractions": "1.0.0",
     "Newtonsoft.Json": "9.0.1"
   },