NLogLoggingConfiguration - Added support for default-wrapper, default-target-parameter & autoReload #283

snakefoot · 2019-04-03T21:20:25Z

and default-target-parameters:

Example JSON-config:

{
    "NLog": {
        "targets": {
            "default-wrapper": {
                "type": "AsyncWrapper",
                "overflowAction": "Block"
            },
            "default-target-parameters": {
                "console": {
                    "error": "true",
                },
                "file": {
                    "encoding": "utf-8",
                },
            },
            "console": {
                "type": "Console"
            },
            "debugLog": {
                "type": "File",
                "fileName": "c:/temp/console-debug.log"
            },
            "errorLog": {
                "type": "File",
                "fileName": "c:/temp/console-error.log"
            }
        },
        "rules": [
            {
                "logger": "*",
                "minLevel": "Trace",
                "writeTo": "debugLog,Console"
            },
            {
                "logger": "*",
                "minLevel": "Error",
                "writeTo": "errorLog"
            }
        ]
    }
}

…ult-target-parameters

codecov · 2019-04-03T21:30:48Z

Codecov Report

Merging #283 into master will increase coverage by 0.33%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   70.45%   70.79%   +0.33%     
==========================================
  Files          12       12              
  Lines         958      976      +18     
  Branches      170      178       +8     
==========================================
+ Hits          675      691      +16     
  Misses        221      221              
- Partials       62       64       +2

Impacted Files	Coverage Δ
...ensions.Logging/Config/NLogLoggingConfiguration.cs	`85.1% <90.9%> (+0.89%)`	⬆️

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 4f33779...0c645f6. Read the comment docs.

snakefoot · 2019-04-03T21:32:21Z

@304NotModified One more bullet in the chamber. Do we have other frequently used config-features?

304NotModified · 2019-04-03T22:05:39Z

Have to check. One in mind, Do we support async=true on targets level?

304NotModified · 2019-04-03T22:07:39Z

I was thinking the default-wrapper etc should be outside the wrappers (so next to it, instead of inside). Not sure of that's problematic (could image that!). If so, then is not a problem

snakefoot · 2019-04-03T22:08:54Z

I was thinking the default-wrapper etc should be outside the wrappers (so next to it, instead of inside).

When you say "wrappers" then you mean "targets"-section ? For xml-config then default-wrapper lives inside the targets-element.

Xml-config has support for multiple "targets"-sections with different "default-wrapper" and "default-target-parameters". Right now I'm focusing on a single "targets"-section since 99 out 100 are doing this.

snakefoot · 2019-04-03T22:10:12Z

Do we support async=true on targets level?

I was hoping one could just do this:

            "default-wrapper": {
                "type": "AsyncWrapper"
            },

304NotModified · 2019-04-03T22:13:24Z

When you say "wrappers" then you mean "targets"-section ? For xml-config then default-wrapper lives inside the targets-element.

Yes and yes I know : 👼

snakefoot · 2019-04-03T22:15:01Z

@304NotModified Yes and yes I know

I'm trying to make the JSON and XML config match as much as possible, unless there is a more natural JSON-formating (Ex. making use of JSON-dictionary-keys)

I feel that default-target-parameters and default-wrapper belongs inside the targets-section.

Maybe one day JSON-config will be used with a targets-sections-array (very exotic and not sure if it is support by the current logic)

        "targets": [
            {
               "console": {
                   "type": "Console"
               }
            },
            {
                "default-wrapper": {
                   "type": "AsyncWrapper",
                   "overflowAction": "Block"
                },
                "debugLog": {
                   "type": "File",
                   "fileName": "c:/temp/console-debug.log"
                }
            }
        ]

304NotModified · 2019-04-03T22:29:28Z

I'm trying to make the JSON and XML config match as much as possible

Why is that? As xml and json have different conventions and limitations. I really prefer natural json over xml-with-json-sause.

Of course, if this is too difficult in the code (for now), that's fine.

304NotModified · 2019-04-03T22:31:02Z

PS. In the json there is now a possible name clash, and not in the xml version.

snakefoot · 2019-04-03T22:35:18Z

I really prefer natural json over xml-with-json-sause.

I feel that I have been very diligent in trying to make the Json-format as pretty as possible. And the reason for making it close to xml-naming and xml-layout is because of documentation reuse. Allows the user to quickly map back and forth (And use the existing wiki). If you feel that the my choices in the Json-format are completely wack then we should probably rollback everything. Then you can start over.

PS. In the json there is now a possible name clash, and not in the xml version.

Would be very obfuscating if someone started to name their actual targets "default-wrapper" or "default-target-parameters". Even in xml-version.

304NotModified · 2019-04-03T22:38:19Z

I feel that I have been very diligent

Yes you are, sorry for the used words. Its not meant unkind.

304NotModified · 2019-04-14T18:41:54Z

How much work would it be to get the new elements (default-wrapper,default-target-parameters) on level up?

e.g.

{
    "NLog": {
         "default-wrapper": {
            "type": "AsyncWrapper",
            "overflowAction": "Block"
         },
         "default-target-parameters": {
             "console": {
                 "error": "true",
             },
             "file": {
                 "encoding": "utf-8",
             },
         },
        "targets": {
           
            "console": {
                "type": "Console"
            },
            "debugLog": {
                "type": "File",
                "fileName": "c:/temp/console-debug.log"
            },
            "errorLog": {
                "type": "File",
                "fileName": "c:/temp/console-error.log"
            }
        },
        "rules": [
            {
                "logger": "*",
                "minLevel": "Trace",
                "writeTo": "debugLog,Console"
            },
            {
                "logger": "*",
                "minLevel": "Error",
                "writeTo": "errorLog"
            }
        ]
    }
}

That would make this change even better IMO

snakefoot · 2019-04-14T21:45:14Z

get the new elements (default-wrapper,default-target-parameters) on level up?

They are not new elements. They exists already in NLog-config-engine. Just feeding them properly so NLog can understand the Json-version.

That would make this change even better IMO

Everything is possible with code. It is just gonna be very ugly. Have to inject the "special" children ( "default-wrapper" + "default-target-parameters") from the top-element into its targets-child.

snakefoot · 2019-04-14T23:16:15Z

@304NotModified After having looked the code. Then I think I will let you have the honors of trying to bend the LoggingConfigurationParser to your will.

…ry integration

codecov-io · 2019-04-14T23:19:15Z

Codecov Report

Merging #283 into master will increase coverage by 4.46%.
The diff coverage is 95.23%.

@@            Coverage Diff            @@
##           master    #283      +/-   ##
=========================================
+ Coverage   69.23%   73.7%   +4.46%     
=========================================
  Files          11      12       +1     
  Lines         933    1080     +147     
  Branches      165     189      +24     
=========================================
+ Hits          646     796     +150     
+ Misses        226     220       -6     
- Partials       61      64       +3

Impacted Files	Coverage Δ
...ensions.Logging/Config/NLogLoggingConfiguration.cs	`93.43% <95.23%> (+9.22%)`	⬆️
...g.Extensions.Logging/Logging/NLogLoggerProvider.cs	`75.38% <0%> (-1.54%)`	⬇️
...tensions.Hosting/Extensions/ConfigureExtensions.cs	`92% <0%> (ø)`
src/NLog.Extensions.Logging/Logging/NLogLogger.cs	`81.54% <0%> (+1.1%)`	⬆️
...tensions.Logging/Extensions/ConfigureExtensions.cs	`14.51% <0%> (+6.45%)`	⬆️

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 2b9b021...d620865. Read the comment docs.

304NotModified · 2019-04-27T19:01:06Z

@304NotModified After having looked the code. Then I think I will let you have the honors of trying to bend the LoggingConfigurationParser to your will.

-> snakefoot#7

304NotModified · 2019-04-27T19:28:24Z

note: I think there is something wrong with the casing, #283 (comment)

shows "targets", but It's implemented as "Targets"?

Default wrapper and Default-target-parameters on top level

snakefoot · 2019-04-27T19:30:06Z

Sure hope everything is case-insensitive, else it would become nightmare.

snakefoot · 2019-04-27T19:31:16Z

Lots of CodeFactor issues now after merge :)

304NotModified · 2019-04-27T19:34:18Z

Lots of CodeFactor issues now after merge :)

I will fix that (soon)

304NotModified · 2019-04-27T20:06:11Z

Sure hope everything is case-insensitive, else it would become nightmare.

do we need to check/test that?

snakefoot · 2019-04-27T20:09:22Z

do we need to check/test that?

Should be handled automatically by LoggingConfigurationParser. Unless there is some case-sensitive logic in NLogLoggingConfiguration.

304NotModified · 2019-04-27T20:11:21Z

Should be handled automatically by LoggingConfigurationParser. Unless there is some case-sensitive logic in NLogLoggingConfiguration.

changing NLogLoggingConfigurationTests, e.g. memoryConfig["NLog:Targets:file:type"] to memoryConfig["NLog:targets:file:type"] fails the test, is that correct?

snakefoot · 2019-04-27T20:11:39Z

According to:

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-2.2

Keys are case-insensitive. For example, ConnectionString and connectionstring are treated as equivalent keys.

So calls to GetSection should work correctly independent of casing.

snakefoot · 2019-04-27T20:12:41Z

changing NLogLoggingConfigurationTests, e.g. memoryConfig["NLog:Targets:file:type"] to memoryConfig["NLog:targets:file:type"] fails the test, is that correct?

Nope it should not fail.

304NotModified · 2019-04-27T20:13:29Z

Nope it should not fail.

I see the issue I think

304NotModified · 2019-04-27T20:14:50Z

37abc56

but maybe doubting if this is a good change

snakefoot · 2019-04-27T20:16:08Z

I can change the casing without tests breaking. But running on my clean PR without your changes (And without case-insensitive-key-comparer on dictionary)

snakefoot · 2019-04-27T20:30:39Z

But since you have identified a possible issue with breaking case-insensitivity, then it could be a good idea to have a test of this.

304NotModified · 2019-04-27T20:31:41Z

yes, added some (in the current tests 👼 )

and this goes wrong: (not committed)

 memoryConfig["NLog:rules:0:logger"] = "*";

(lower case rules)

this works

memoryConfig["nlog:Rules:0:logger"] = "*";

(lower case nlog)

304NotModified · 2019-04-27T20:34:07Z

and this goes wrong: (not committed)
 memoryConfig["NLog:rules:0:logger"] = "*";
(lower case rules)

Any idea why? Just checked the NLog dev branch, but don't see the issue 123.

snakefoot · 2019-04-27T20:34:26Z

and this goes wrong: memoryConfig["NLog:rules:0:logger"] = "*";

Think you are now testing the test. Make sure to have all entries to have the same casing. Ex:

            memoryConfig["NLog:rules:0:logger"] = "*";
            memoryConfig["NLog:rules:0:minLevel"] = "Trace";
            memoryConfig["NLog:rules:0:writeTo"] = "File,Console";

304NotModified · 2019-04-27T20:38:20Z

ow otherwise those are separate sections? (rules and Rules)?

snakefoot · 2019-04-27T20:39:26Z

Could you change your "casing"-test so all entries are using the same casing ?

304NotModified · 2019-04-27T21:07:59Z

But it won't break anything?

snakefoot · 2019-04-27T21:09:33Z

But it won't break anything?

Not sure I understand. Anyway will create a new PR when this is merged, since support for KeepVariablesOnReload is still missing

304NotModified · 2019-04-27T21:33:33Z

Could you change your "casing"-test so all entries are using the same casing ?

done

304NotModified · 2019-04-27T22:03:33Z

merged :)

NLogLoggingConfiguration - Added support for default-wrapper and defa…

0c645f6

…ult-target-parameters

NLogLoggingConfiguration - Added support for AutoReload with LogFacto…

5029716

…ry integration

snakefoot force-pushed the master branch from 24579a2 to 5029716 Compare April 14, 2019 23:19

snakefoot mentioned this pull request Apr 26, 2019

LoggingConfiguration - Rollback static Reload-method NLog/NLog#3335

Merged

304NotModified added 2 commits April 27, 2019 20:53

Default wrapper and default target on root level

5ae816a

context

1a7462b

304NotModified mentioned this pull request Apr 27, 2019

Default wrapper and Default-target-parameters on top level snakefoot/NLog.Extensions.Logging#7

Merged

Merge pull request #7 from NLog/default-wrapper2

592c8d0

Default wrapper and Default-target-parameters on top level

304NotModified self-assigned this Apr 27, 2019

refactor

d19cf39

fix MemoryConfig

37abc56

304NotModified added 2 commits April 27, 2019 22:20

small refactor

e1c50a2

fixup

feb31c9

casing tests

acd9b2f

304NotModified added the feature label Apr 27, 2019

304NotModified added this to the 1.5 milestone Apr 27, 2019

revert casing

d620865

304NotModified changed the title ~~NLogLoggingConfiguration - Added support for default-wrapper and Default-target-parameter~~ NLogLoggingConfiguration - Added support for default-wrapper, default-target-parameter & AutoReload Apr 27, 2019

304NotModified changed the title ~~NLogLoggingConfiguration - Added support for default-wrapper, default-target-parameter & AutoReload~~ NLogLoggingConfiguration - Added support for default-wrapper, default-target-parameter & autoReload Apr 27, 2019

304NotModified merged commit cef73eb into NLog:master Apr 27, 2019

NLogLoggingConfiguration - Added support for default-wrapper, default-target-parameter & autoReload #283

NLogLoggingConfiguration - Added support for default-wrapper, default-target-parameter & autoReload #283

Conversation

snakefoot commented Apr 3, 2019 • edited Loading

codecov bot commented Apr 3, 2019 • edited Loading

Codecov Report

snakefoot commented Apr 3, 2019 • edited Loading

304NotModified commented Apr 3, 2019

304NotModified commented Apr 3, 2019 • edited Loading

snakefoot commented Apr 3, 2019 • edited Loading

snakefoot commented Apr 3, 2019 • edited Loading

304NotModified commented Apr 3, 2019

snakefoot commented Apr 3, 2019 • edited Loading

304NotModified commented Apr 3, 2019

304NotModified commented Apr 3, 2019

snakefoot commented Apr 3, 2019 • edited Loading

304NotModified commented Apr 3, 2019

304NotModified commented Apr 14, 2019

snakefoot commented Apr 14, 2019 • edited Loading

snakefoot commented Apr 14, 2019

codecov-io commented Apr 14, 2019 • edited Loading

Codecov Report

304NotModified commented Apr 27, 2019

304NotModified commented Apr 27, 2019

snakefoot commented Apr 27, 2019 • edited Loading

snakefoot commented Apr 27, 2019

304NotModified commented Apr 27, 2019

304NotModified commented Apr 27, 2019

snakefoot commented Apr 27, 2019

304NotModified commented Apr 27, 2019

snakefoot commented Apr 27, 2019

snakefoot commented Apr 27, 2019

304NotModified commented Apr 27, 2019

304NotModified commented Apr 27, 2019

snakefoot commented Apr 27, 2019 • edited Loading

snakefoot commented Apr 27, 2019

304NotModified commented Apr 27, 2019 • edited Loading

304NotModified commented Apr 27, 2019

snakefoot commented Apr 27, 2019

304NotModified commented Apr 27, 2019

snakefoot commented Apr 27, 2019

304NotModified commented Apr 27, 2019

snakefoot commented Apr 27, 2019

304NotModified commented Apr 27, 2019

304NotModified commented Apr 27, 2019

snakefoot commented Apr 3, 2019 •

edited

Loading

codecov bot commented Apr 3, 2019 •

edited

Loading

snakefoot commented Apr 3, 2019 •

edited

Loading

304NotModified commented Apr 3, 2019 •

edited

Loading

snakefoot commented Apr 3, 2019 •

edited

Loading

snakefoot commented Apr 3, 2019 •

edited

Loading

snakefoot commented Apr 3, 2019 •

edited

Loading

snakefoot commented Apr 3, 2019 •

edited

Loading

snakefoot commented Apr 14, 2019 •

edited

Loading

codecov-io commented Apr 14, 2019 •

edited

Loading

snakefoot commented Apr 27, 2019 •

edited

Loading

snakefoot commented Apr 27, 2019 •

edited

Loading

304NotModified commented Apr 27, 2019 •

edited

Loading