Skip to content
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

Added NLogBuilder and ASP.NET Core 2 example #191

Merged
merged 3 commits into from
Oct 12, 2017

Conversation

304NotModified
Copy link
Member

@304NotModified 304NotModified commented Oct 7, 2017

@codecov
Copy link

codecov bot commented Oct 7, 2017

Codecov Report

Merging #191 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #191   +/-   ##
=====================================
  Coverage      58%    58%           
=====================================
  Files          29     29           
  Lines         386    386           
  Branches       93     93           
=====================================
  Hits          224    224           
  Misses        126    126           
  Partials       36     36

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 1509356...1623e85. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 7, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #191   +/-   ##
=====================================
- Coverage      58%    58%   -1%     
=====================================
  Files          29     29           
  Lines         386    386           
  Branches       93     93           
=====================================
- Hits          224    222    -2     
  Misses        126    126           
- Partials       36     38    +2
Impacted Files Coverage Δ
...youtRenderers/AspNetRequestCookieLayoutRenderer.cs 81% <0%> (-4%) ⬇️
...outRenderers/AspNetLayoutMultiValueRendererBase.cs 95% <0%> (-2%) ⬇️

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 1509356...51f8969. Read the comment docs.

added NLogAspNetCoreOptions

auto register:  <extensions> not needed anymore

fix namespaces

docs
public static void Main(string[] args)
{
// NLog: setup the logger first to catch all errors
var logger = NLogStart.InitLogger("NLog.config");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snakefoot inspiration for a better names?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitLogger is very confusing. It almost seem like you are creating a logger named "NLog.config".

Maybe rename NLogStart to NLogBuilder, and rename "InitLogger" to "LoadConfig" where LoadConfig returns NLogBuilder (fluent interface like)

NLogBuilder.LoadConfig("NLog.config").GetCurrentClassLogger()

NLogBuilder.GetCurrentClassLogger()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware that calling GetCurrentClassLogger within NLogBuilder will capture the wrong classname.

Copy link
Member Author

@304NotModified 304NotModified Oct 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NLogBuilder.LoadConfig("NLog.config").GetCurrentClassLogger()

LoadConfig could return the LogFactory, not sure it that's clear. What do you think?

also doubting to call it ConfigureNLog, that consistent with the ConfigureNLog on ILoggingBuilder, (see #185) but maybe consistent is just confusing here.

Copy link
Contributor

@snakefoot snakefoot Oct 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure it that's clear. What do you think?

Actually what you want is just the LogManager-interface, but you want people to perform the registration of AspNetExtensions before loading the config (From file or programmatically). Think it would be easier for all NLog-users if LogManager remained the default goto-place.

Think you want something like ConfigureNLog that just configures everything, and then people can use LogManager from there.

Maybe you could change the RegisterExtendedItems() to attempt loading known good NLog-assemblies. Like NLog.Extended.dll and NLog.Web.dll and NLog.Web.AspNetCore.dll (Maybe it could do some probing of the environment, to detect if there is a high-chance of succesfully loading one of these dlls)

Usually this auto-loading of NLog-dlls works out-of-the-box, but not when using Nuget-package-dlls that reside in different sub-folder.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's dull about it?

Very hard to call GetCurrentClassLogger() or Flush() on the LoggingConfiguration-object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well Flush isn't needed in this case? Or do you think it's wise to add it after BuildWebHost?

Copy link
Contributor

@snakefoot snakefoot Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can just see that people like the fluent-interface of Serilog, and it requires that you have access to a "powerful" object like the LogFactoy. But yes normally one will just be calling GetCurrentClassLogger().

But I prefer to have 3 stages. Startup - Running - Shutdown. Where Startup is where I load the NLog-config and the Running-stage where I acquire my loggers. Each stage happens at different locations in the code, so I don't need to build the nlog-config and get the logger-interface at the same location. I certainly don't want to load the nlog-config multiple times, only once!

I see serilog fixes this by using something called context, so the same global-logger is used everywhere but one can apply multiple local-context-names at different locations. But I kind of like the NLog-style of the logger-name being captured from the context-class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We load it once here?

Fluent config would be great indeed. With the json config feature, that's one of the next steps for 4.6/4.7 I think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We load it once here?

Yes but one might think that if I need to get a logger at different place, then I should just do the same (Call LoadConfig/ConfigureNLog and then GetCurrentClassLogger), and then one might be surprised by the side-effects of the config-reload.

Or one might be cheated by the NLog-singleton-behavior, as it is hard to have multiple configs at the same time.

public static void Main(string[] args)
{
// NLog: setup the logger first to catch all errors
var logger = NLogBuilder.ConfigureNLog("NLog.config").GetCurrentClassLogger();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snakefoot updated it! I think is indeed a lot better.

@grokky1 any opinion on this? Do you think this is clear?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest changes are good. Looking forward to using this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grokky1 the beta is live. Please let me know of you're happy, rtm will be soon then. See also.https://github.com/NLog/NLog.Web/wiki/Getting-started-with-ASP.NET-Core-2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@304NotModified Sorry busy wrapping up a sprint so didn't get a chance, will try soon!!

@304NotModified 304NotModified changed the title Added InitLogger and ASP.NET Core 2 example Added NLogBuilder and ASP.NET Core 2 example Oct 11, 2017
internal logging
@304NotModified 304NotModified merged commit 41da1c0 into master Oct 12, 2017
@304NotModified 304NotModified deleted the example-aspnetcore2 branch October 12, 2017 14:06
@snakefoot snakefoot added ASP.NET Core ASP.NET Core - all versions and removed ASP.NET Core 2 labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASP.NET Core ASP.NET Core - all versions feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make <add assembly="NLog.Web.AspNetCore"/> unneeded
3 participants