-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configure NLogProviderOptions from appsettings.json #248
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
"Logging": { | ||
"NLog": { | ||
"IncludeScopes": false, | ||
"ParseMessageTemplates": true, | ||
"CaptureMessageProperties": true | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,7 +41,17 @@ public static IHostBuilder UseNLog(this IHostBuilder builder, NLogProviderOption | |
ConfigurationItemFactory.Default.RegisterItemsFromAssembly(typeof(ConfigureExtensions).GetTypeInfo() | ||
.Assembly); | ||
|
||
services.AddSingleton(new LoggerFactory().AddNLog(options)); | ||
services.AddSingleton<ILoggerProvider>(serviceProvider => | ||
{ | ||
var provider = new NLogLoggerProvider(options ?? new NLogProviderOptions()); | ||
if (hostbuilder.Configuration != null) | ||
{ | ||
// TODO ConfigSettingLayoutRenderer.DefaultConfiguration = hostbuilder.Configuration; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when should be do this TODO? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Next PR. You just requested me to split this into more PRs |
||
if (options == null) | ||
provider.Configure(hostbuilder.Configuration?.GetSection("Logging:NLog")); | ||
} | ||
return provider; | ||
}); | ||
}); | ||
|
||
return builder; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,30 @@ | ||
using System; | ||
using System.Linq; | ||
using System.Reflection; | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Extensions.Logging; | ||
using NLog.Common; | ||
using NLog.Config; | ||
|
||
namespace NLog.Extensions.Logging | ||
{ | ||
/// <summary> | ||
/// Helpers for .NET Core | ||
/// Helpers for configuring NLog for Microsoft Extension Logging (MEL) | ||
/// </summary> | ||
public static class ConfigureExtensions | ||
{ | ||
/// <summary> | ||
/// Enable NLog as logging provider in .NET Core. | ||
/// Enable NLog as logging provider for Microsoft Extension Logging | ||
/// </summary> | ||
/// <param name="factory"></param> | ||
/// <returns>ILoggerFactory for chaining</returns> | ||
public static ILoggerFactory AddNLog(this ILoggerFactory factory) | ||
{ | ||
return AddNLog(factory, null); | ||
return factory.AddNLog(NLogProviderOptions.Default); | ||
} | ||
|
||
/// <summary> | ||
/// Enable NLog as logging provider in .NET Core. | ||
/// Enable NLog as logging provider for Microsoft Extension Logging | ||
/// </summary> | ||
/// <param name="factory"></param> | ||
/// <param name="options">NLog options</param> | ||
|
@@ -33,19 +35,45 @@ public static ILoggerFactory AddNLog(this ILoggerFactory factory, NLogProviderOp | |
return factory; | ||
} | ||
|
||
/// <summary> | ||
/// Enable NLog as logging provider for Microsoft Extension Logging | ||
/// </summary> | ||
/// <param name="factory"></param> | ||
/// <param name="configuration"></param> | ||
/// <returns>ILoggerFactory for chaining</returns> | ||
public static ILoggerFactory AddNLog(this ILoggerFactory factory, IConfiguration configuration) | ||
{ | ||
var provider = CreateNLogProvider(configuration); | ||
factory.AddProvider(provider); | ||
return factory; | ||
} | ||
|
||
#if !NETCORE1_0 | ||
/// <summary> | ||
/// Enable NLog as logging provider in .NET Core. | ||
/// Enable NLog as logging provider for Microsoft Extension Logging | ||
/// </summary> | ||
/// <param name="factory"></param> | ||
/// <returns>ILoggerFactory for chaining</returns> | ||
public static ILoggingBuilder AddNLog(this ILoggingBuilder factory) | ||
{ | ||
return AddNLog(factory, null); | ||
return factory.AddNLog(NLogProviderOptions.Default); | ||
} | ||
|
||
/// <summary> | ||
/// Enable NLog as logging provider in .NET Core. | ||
/// Enable NLog as logging provider for Microsoft Extension Logging | ||
/// </summary> | ||
/// <param name="factory"></param> | ||
/// <param name="configuration">Configuration</param> | ||
/// <returns>ILoggerFactory for chaining</returns> | ||
public static ILoggingBuilder AddNLog(this ILoggingBuilder factory, IConfiguration configuration) | ||
{ | ||
var provider = CreateNLogProvider(configuration); | ||
factory.AddProvider(provider); | ||
return factory; | ||
} | ||
|
||
/// <summary> | ||
/// Enable NLog as logging provider for Microsoft Extension Logging | ||
/// </summary> | ||
/// <param name="factory"></param> | ||
/// <param name="options">NLog options</param> | ||
|
@@ -83,5 +111,48 @@ public static LoggingConfiguration ConfigureNLog(this ILoggerFactory loggerFacto | |
LogManager.Configuration = config; | ||
return config; | ||
} | ||
|
||
/// <summary> | ||
/// Factory method for <see cref="NLogLoggerProvider"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this isn't a factory method? (IMO factory method should be called "Create") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope it is an extension-method with fluent-interface. |
||
/// </summary> | ||
/// <param name="nlogProvider"></param> | ||
/// <param name="configurationSection">Microsoft Extension Configuration</param> | ||
/// <returns></returns> | ||
public static NLogLoggerProvider Configure(this NLogLoggerProvider nlogProvider, IConfigurationSection configurationSection) | ||
{ | ||
if (configurationSection == null) | ||
return nlogProvider; | ||
|
||
var configProps = nlogProvider.Options.GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public).Where(p => p.SetMethod?.IsPublic == true).ToDictionary(p => p.Name, StringComparer.OrdinalIgnoreCase); | ||
foreach (var configValue in configurationSection.GetChildren()) | ||
{ | ||
if (configProps.TryGetValue(configValue.Key, out var propertyInfo)) | ||
{ | ||
try | ||
{ | ||
var result = Convert.ChangeType(configValue.Value, propertyInfo.PropertyType); | ||
propertyInfo.SetMethod.Invoke(nlogProvider.Options, new[] { result }); | ||
} | ||
catch (Exception ex) | ||
{ | ||
InternalLogger.Warn(ex, "NLogProviderOptions: Property {0} could not be assigned value: {1}", configValue.Key, configValue.Value); | ||
} | ||
} | ||
} | ||
|
||
return nlogProvider; | ||
} | ||
|
||
private static NLogLoggerProvider CreateNLogProvider(IConfiguration configuration) | ||
{ | ||
var provider = new NLogLoggerProvider(new NLogProviderOptions()); | ||
if (configuration != null) | ||
{ | ||
// TODO ConfigSettingLayoutRenderer.DefaultConfiguration = configuration; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this TODO for this PR, or do we need a Github issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Next PR. You just requested me to split this into more PRs |
||
provider.Configure(configuration.GetSection("Logging:NLog")); | ||
} | ||
|
||
return provider; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is DI-proof.
I think it's a good change to move to the Factory instead of Provider, that's the one we could also bootstrap from DI. See #253
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain DI-proof? Adding the NLogLoggerProvider as provider to the official LoggerFactory is the nice way of being part of Microsoft Logging Extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like to change the call, if we need a new dependency. But is a bit debatable if the config is here an dependency or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning that NLogLoggingProvider.Options and NLog-LoggingConfiguration was loaded from IConfiguration. So it would be natural to receive the config when doing AddNLog.
But not that skilled with DI and fluent-interface, so any help with creating a great interface is very welcome.