Configure NLogProviderOptions from appsettings.json #248

snakefoot · 2018-09-22T11:56:34Z

Added support for configuring NLogProviderOptions from appsettings.json (Updated package "Microsoft.Extensions.Logging" from Version="2.0.1" to Version="2.1.0")

Added new NLogLoggerProvider-constructor with LogFactory-parameter

304NotModified · 2018-09-22T12:05:18Z

I was just preparing 1.3 ;) #247

304NotModified · 2018-09-22T12:07:34Z

I don't fully understand what this PR is changing. What's the benefit here?

src/NLog.Extensions.Configuration/ConfigSettingLayoutRenderer.cs

codecov · 2018-09-22T12:11:01Z

Codecov Report

Merging #248 into master will decrease coverage by 12.13%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #248       +/-   ##
===========================================
- Coverage   70.19%   58.06%   -12.14%     
===========================================
  Files           9        1        -8     
  Lines         510       62      -448     
  Branches      129       17      -112     
===========================================
- Hits          358       36      -322     
+ Misses         99       15       -84     
+ Partials       53       11       -42

Impacted Files	Coverage Δ
...nsions.Logging/Logging/NLogMessageParameterList.cs
....Extensions.Logging/Logging/NLogProviderOptions.cs
src/NLog.Extensions.Logging/Logging/NLogLogger.cs
...og.Extensions.Logging/Logging/NLogLoggerFactory.cs
...Extensions.Logging/Logging/NLogBeginScopeParser.cs

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a841c...87369ca. Read the comment docs.

src/NLog.Extensions.Configuration/NLog.Extensions.Configuration.csproj

snakefoot · 2018-09-22T12:12:44Z

I don't fully understand what this PR is changing. What's the benefit here?

Not a big AspNet-Core-Expert, but I see lots of people doing this:

   var builder = new ConfigurationBuilder()
        .SetBasePath(env.ContentRootPath)
        .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
        .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true)
        .AddEnvironmentVariables();

Trying to make this layoutrenderer do the same when default configuration.

304NotModified · 2018-09-22T12:13:40Z

shouldn't we do .SetBasePath(env.ContentRootPath) then always?

snakefoot · 2018-09-22T12:14:08Z

shouldn't we do .SetBasePath(env.ContentRootPath) then always?

Where do I get the hosting environment? Think AppDomain.BaseDirectory is just fine.

304NotModified · 2018-09-22T12:17:13Z

Where do I get the hosting environment?

Good one.

And always AppDomain.BaseDirectory then? I think it's a bit magic this only done with the "default config". Or is that a breaking (behavior) change?

snakefoot · 2018-09-22T12:18:04Z

Btw. going out now, so will not be able to make any changes. If you need more modifications, then you to push them yourself

Or is that a breaking (behavior) change?

Breaking change from what?

304NotModified · 2018-09-22T13:51:10Z

OK have fun

304NotModified · 2018-09-22T13:54:54Z

I can't push on your master :|

snakefoot · 2018-09-22T16:28:36Z

@304NotModified What changes do you like me to make?

src/NLog.Extensions.Configuration/ConfigSettingLayoutRenderer.cs

304NotModified · 2018-10-11T20:59:23Z

examples/NetCore2/ConsoleExample/Program.cs

-                CaptureMessageProperties = true
+                loggingBuilder.ClearProviders();
+                loggingBuilder.SetMinimumLevel(Microsoft.Extensions.Logging.LogLevel.Trace);
+                loggingBuilder.AddNLog(config);


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

Please explain DI-proof? Adding the NLogLoggerProvider as provider to the official LoggerFactory is the nice way of being part of Microsoft Logging Extensions.

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.

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.

src/NLog.Extensions.Logging/Logging/NLogLoggerProvider.cs

304NotModified · 2018-12-28T23:40:19Z

I'm sorry this PR is still open. It's a bit too big for me to handle currently.

304NotModified · 2019-01-12T10:25:11Z

Could we please split this PR into multiple? It's far more easier to read/review/test then.

snakefoot · 2019-01-12T13:18:43Z

Now it just adds support for configuring NLogProviderOptions from appsettings.json. And the ability to provide isolated LogFactory-object.

When review complete, then we take the next code-change (Merging NLog.Extension.Configuration into NLog.Extension.Logging).

304NotModified · 2018-10-16T18:33:55Z

src/NLog.Extensions.Logging/Extensions/ConfigureExtensions.cs

@@ -83,5 +153,48 @@ public static LoggingConfiguration ConfigureNLog(this ILoggerFactory loggerFacto
            LogManager.Configuration = config;
            return config;
        }
+
+        /// <summary>
+        /// Factory method for <see cref="NLogLoggerProvider"/>


this isn't a factory method? (IMO factory method should be called "Create")

Nope it is an extension-method with fluent-interface.

304NotModified · 2018-10-16T18:35:12Z

src/NLog.Extensions.Logging/Extensions/ConfigureExtensions.cs

+        /// <param name="nlogProvider"></param>
+        /// <param name="configuration">Microsoft Extension Configuration</param>
+        /// <returns></returns>
+        public static NLogLoggerProvider ConfigureNLogProvider(this NLogLoggerProvider nlogProvider, IConfiguration configuration)


Proposal: Configure

304NotModified · 2019-01-13T00:22:26Z

src/NLog.Extensions.Hosting/Extensions/ConfigureExtensions.cs

+                    var provider = new NLogLoggerProvider(options ?? new NLogProviderOptions());
+                    if (hostbuilder.Configuration != null)
+                    {
+                        // TODO ConfigSettingLayoutRenderer.DefaultConfiguration = hostbuilder.Configuration;


when should be do this TODO?

Next PR. You just requested me to split this into more PRs

304NotModified · 2019-01-13T00:23:15Z

src/NLog.Extensions.Logging/Extensions/ConfigureExtensions.cs

@@ -33,6 +35,19 @@ public static ILoggerFactory AddNLog(this ILoggerFactory factory, NLogProviderOp
            return factory;
        }

+        /// <summary>
+        /// Enable NLog as logging provider in .NET Core.


not sure if this should be ".NET Standard." instead of ".NET Core"

NetCore is the platform. NetStandard is the API-specification.

But yes lets call it Net Standard since this packages works on both NetCore and NetFramework.

Have updated all comments to say "for Microsoft Extension Logging".

304NotModified · 2019-01-13T00:24:58Z

src/NLog.Extensions.Logging/Extensions/ConfigureExtensions.cs

+            var provider = new NLogLoggerProvider(new NLogProviderOptions());
+            if (configuration != null)
+            {
+                // TODO ConfigSettingLayoutRenderer.DefaultConfiguration = configuration;


is this TODO for this PR, or do we need a Github issue?

Next PR. You just requested me to split this into more PRs

304NotModified · 2019-01-13T00:27:47Z

test/NLog.Extensions.Logging.Tests/CustomBeginScopeTest.cs

@@ -40,7 +40,7 @@ public void TestNonSerializableSayHi()
            var scopeString = runner.SayHi().Result;
            Assert.Single(target.Logs);
            Assert.Equal("Hi Earth. Welcome Earth People", target.Logs[0]);
-            Assert.Equal("Earth People", scopeString);
+            // Assert.Equal("Earth People", scopeString); <-- Bug https://github.com/aspnet/Logging/issues/893


Dunno if this is solved yet, as the repo has been changed/moved (now readonly)

Think it has been resolved in NetCore2.2, but that is not the long-term-support-version (NetCore 2.1 is)

snakefoot force-pushed the master branch 2 times, most recently from 02f789d to 227b64f Compare September 22, 2018 12:00

snakefoot mentioned this pull request Sep 22, 2018

Introduced NLog.AppConfig for .NET 45 + .NET Standard 2.0 NLog/NLog#2919

Closed

snakefoot force-pushed the master branch from 227b64f to d2f4553 Compare September 22, 2018 12:02

304NotModified reviewed Sep 22, 2018

View reviewed changes

src/NLog.Extensions.Configuration/ConfigSettingLayoutRenderer.cs Outdated Show resolved Hide resolved

snakefoot force-pushed the master branch from d2f4553 to 8964924 Compare September 22, 2018 12:10

304NotModified requested changes Sep 22, 2018

View reviewed changes

src/NLog.Extensions.Configuration/NLog.Extensions.Configuration.csproj Outdated Show resolved Hide resolved

snakefoot force-pushed the master branch from 8964924 to 547d5a1 Compare September 22, 2018 12:13

304NotModified added this to the 1.3.1 milestone Sep 22, 2018

304NotModified reviewed Sep 22, 2018

View reviewed changes

src/NLog.Extensions.Configuration/ConfigSettingLayoutRenderer.cs Outdated Show resolved Hide resolved

snakefoot force-pushed the master branch 2 times, most recently from c1de8a0 to 431dd5d Compare September 23, 2018 07:53

snakefoot changed the title ~~configsetting - Include AddEnvironmentVariables when default config~~ configsetting - Remove EnvironmentVariables dependency Sep 23, 2018

snakefoot force-pushed the master branch from 5e17c9b to c12c656 Compare September 27, 2018 15:03

NLogLoggerProvider - Added constructor with isolated NLog LogFactory

8f2a7c4

snakefoot force-pushed the master branch 4 times, most recently from 1327a8f to 30bb4c7 Compare September 27, 2018 20:32

snakefoot force-pushed the master branch from ec2b4c0 to 8670280 Compare October 11, 2018 17:38

304NotModified reviewed Oct 11, 2018

View reviewed changes

snakefoot mentioned this pull request Oct 13, 2018

Configure from JSON instead of XML NLog/NLog#1588

Closed

snakefoot force-pushed the master branch 2 times, most recently from 87dcae1 to 03c30fd Compare October 21, 2018 20:00

snakefoot mentioned this pull request Oct 28, 2018

Override NLog configuration, when called from integration test. NLog/NLog.Web#289

Closed

snakefoot force-pushed the master branch from 03c30fd to 03bc17a Compare October 28, 2018 09:39

greenmooseSE reviewed Nov 24, 2018

View reviewed changes

src/NLog.Extensions.Logging/Logging/NLogLoggerProvider.cs Show resolved Hide resolved

snakefoot mentioned this pull request Dec 4, 2018

Add package NLog.Extensions.Configuration for configsetting layout renderer NLog/NLog.github.io#78

Merged

snakefoot force-pushed the master branch from 03bc17a to 8f2a7c4 Compare January 12, 2019 13:17

snakefoot changed the title ~~moved ${configsetting} to NLog.Extensions.Logging package. Removed filename property. Updated UseNLog/AddNLog.~~ NLogLoggerProvider - Added constructor with isolated NLog LogFactory Jan 12, 2019

snakefoot force-pushed the master branch 5 times, most recently from 9ed4adf to 1f01704 Compare January 12, 2019 14:08

snakefoot changed the title ~~NLogLoggerProvider - Added constructor with isolated NLog LogFactory~~ Configure NLogProviderOptions from appsettings.json Jan 12, 2019

snakefoot force-pushed the master branch 2 times, most recently from 506a2bc to 3c71b3c Compare January 12, 2019 14:32

304NotModified mentioned this pull request Jan 13, 2019

Support .NET Standard new appsettings NLog/NLog#2334

Closed

304NotModified modified the milestones: 1.3.1, 1.4 Jan 13, 2019

304NotModified reviewed Jan 13, 2019

View reviewed changes

Configure NLogProviderOptions from appsettings.json

9e7f343

snakefoot force-pushed the master branch from 3c71b3c to 9e7f343 Compare January 13, 2019 00:43

Configure NLogProviderOptions from appsettings.json (code review)

87369ca

304NotModified merged commit 86ce1c1 into NLog:master Jan 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure NLogProviderOptions from appsettings.json #248

Configure NLogProviderOptions from appsettings.json #248

snakefoot commented Sep 22, 2018 •

edited

304NotModified commented Sep 22, 2018 •

edited

304NotModified commented Sep 22, 2018

codecov bot commented Sep 22, 2018 •

edited

snakefoot commented Sep 22, 2018

304NotModified commented Sep 22, 2018

snakefoot commented Sep 22, 2018 •

edited

304NotModified commented Sep 22, 2018

snakefoot commented Sep 22, 2018

304NotModified commented Sep 22, 2018

304NotModified commented Sep 22, 2018

snakefoot commented Sep 22, 2018

304NotModified Oct 11, 2018

snakefoot Oct 12, 2018 •

edited

304NotModified Oct 12, 2018

snakefoot Oct 12, 2018 •

edited

304NotModified commented Dec 28, 2018

304NotModified commented Jan 12, 2019

snakefoot commented Jan 12, 2019 •

edited

304NotModified Oct 16, 2018

snakefoot Jan 13, 2019

304NotModified Oct 16, 2018

snakefoot Jan 13, 2019

304NotModified Jan 13, 2019

snakefoot Jan 13, 2019

304NotModified Jan 13, 2019

snakefoot Jan 13, 2019

snakefoot Jan 13, 2019

snakefoot Jan 13, 2019

304NotModified Jan 13, 2019

snakefoot Jan 13, 2019

304NotModified Jan 13, 2019 •

edited

snakefoot Jan 13, 2019

Configure NLogProviderOptions from appsettings.json #248

Configure NLogProviderOptions from appsettings.json #248

Conversation

snakefoot commented Sep 22, 2018 • edited

304NotModified commented Sep 22, 2018 • edited

304NotModified commented Sep 22, 2018

codecov bot commented Sep 22, 2018 • edited

Codecov Report

snakefoot commented Sep 22, 2018

304NotModified commented Sep 22, 2018

snakefoot commented Sep 22, 2018 • edited

304NotModified commented Sep 22, 2018

snakefoot commented Sep 22, 2018

304NotModified commented Sep 22, 2018

304NotModified commented Sep 22, 2018

snakefoot commented Sep 22, 2018

Choose a reason for hiding this comment

snakefoot Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snakefoot Oct 12, 2018 • edited

Choose a reason for hiding this comment

304NotModified commented Dec 28, 2018

304NotModified commented Jan 12, 2019

snakefoot commented Jan 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

304NotModified Jan 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snakefoot commented Sep 22, 2018 •

edited

304NotModified commented Sep 22, 2018 •

edited

codecov bot commented Sep 22, 2018 •

edited

snakefoot commented Sep 22, 2018 •

edited

snakefoot Oct 12, 2018 •

edited

snakefoot Oct 12, 2018 •

edited

snakefoot commented Jan 12, 2019 •

edited

304NotModified Jan 13, 2019 •

edited