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

NLog LogLevels are mismatched. #237

Closed
czema opened this Issue Aug 17, 2015 · 23 comments

Comments

Projects
None yet
10 participants
@czema

czema commented Aug 17, 2015

In NLog, Trace is the lowest level and Debug is the second lowest. In MS Logging, Debug is the lowest and Verbose is second lowest. NLogLoggerProvider.cs::75 needs to be changed.

private global::NLog.LogLevel GetLogLevel(LogLevel logLevel)
    {
        switch (logLevel)
        {
            case LogLevel.Debug: return global::NLog.LogLevel.Trace;
            case LogLevel.Verbose: return global::NLog.LogLevel.Debug;
            case LogLevel.Information: return global::NLog.LogLevel.Info;
            case LogLevel.Warning: return global::NLog.LogLevel.Warn;
            case LogLevel.Error: return global::NLog.LogLevel.Error;
            case LogLevel.Critical: return global::NLog.LogLevel.Fatal;
        }
        return global::NLog.LogLevel.Debug;
    }
@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock Aug 19, 2015

Contributor

Serilog has a similar issue. Both NLog and Serilog have Verbose/Trace being the lowest level while Debug is the second lowest. MF.Logging has Debug being the lowest.

What is the reasoning? Any chance MF.Logging can change this? I realize this could be too late.

I'm used to log4net where there is no Trace/Verbose just Debug so I've been confused for a while.

Contributor

adamhathcock commented Aug 19, 2015

Serilog has a similar issue. Both NLog and Serilog have Verbose/Trace being the lowest level while Debug is the second lowest. MF.Logging has Debug being the lowest.

What is the reasoning? Any chance MF.Logging can change this? I realize this could be too late.

I'm used to log4net where there is no Trace/Verbose just Debug so I've been confused for a while.

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Aug 19, 2015

Contributor

+1 if it is at all possible to consider a change at this point; a bit awful having Verbose => Debug and Debug => Verbose :-)

Contributor

nblumhardt commented Aug 19, 2015

+1 if it is at all possible to consider a change at this point; a bit awful having Verbose => Debug and Debug => Verbose :-)

@czema

This comment has been minimized.

Show comment
Hide comment
@czema

czema Sep 3, 2015

Looks like you are using an older beta of Microsoft.Framework.Logging. Beta-7 doesn't include Serilog. Please see https://github.com/serilog/serilog-framework-logging

As for your particular problem, if you look at the beta-4 version of Serilog you will see that LogLevel.Debug does not have a mapping, you need to use LogVerbose.

czema commented Sep 3, 2015

Looks like you are using an older beta of Microsoft.Framework.Logging. Beta-7 doesn't include Serilog. Please see https://github.com/serilog/serilog-framework-logging

As for your particular problem, if you look at the beta-4 version of Serilog you will see that LogLevel.Debug does not have a mapping, you need to use LogVerbose.

@aggieben

This comment has been minimized.

Show comment
Hide comment
@aggieben

aggieben Sep 3, 2015

@czema sorry - I deleted my comment because I realized I was commenting in the wrong repo (and that my problem not what I thought it was at first blush, as you pointed out).

aggieben commented Sep 3, 2015

@czema sorry - I deleted my comment because I realized I was commenting in the wrong repo (and that my problem not what I thought it was at first blush, as you pointed out).

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Sep 20, 2015

Contributor

@czema @aggieben the Serilog provider moved over to be maintained by the Serilog project: https://github.com/serilog/serilog-framework-logging

The more time I spend with this the more of an annoyance I think it might turn out to be. At RTM, anyone wanting to take advantage of the structured logging support in Microsoft.Framework.Logging is likely to do it via one of the already-existing Serilog sinks (there are many). Having two levels with the same names but different semantics will make this awkward to say the least :-)

Contributor

nblumhardt commented Sep 20, 2015

@czema @aggieben the Serilog provider moved over to be maintained by the Serilog project: https://github.com/serilog/serilog-framework-logging

The more time I spend with this the more of an annoyance I think it might turn out to be. At RTM, anyone wanting to take advantage of the structured logging support in Microsoft.Framework.Logging is likely to do it via one of the already-existing Serilog sinks (there are many). Having two levels with the same names but different semantics will make this awkward to say the least :-)

@davidfowl davidfowl added this to the 1.0.0 backlog milestone Sep 20, 2015

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Sep 20, 2015

Contributor

(Should add, I'm happy to PR this if it's a viable change.)

Contributor

nblumhardt commented Sep 20, 2015

(Should add, I'm happy to PR this if it's a viable change.)

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Sep 20, 2015

Contributor

Another plus 1️⃣ to making Trace/Verbose as the lowest and Debug and 2nd lowest.

Contributor

PureKrome commented Sep 20, 2015

Another plus 1️⃣ to making Trace/Verbose as the lowest and Debug and 2nd lowest.

@304NotModified

This comment has been minimized.

Show comment
Hide comment
@304NotModified

304NotModified Oct 10, 2015

Contributor

+1

Contributor

304NotModified commented Oct 10, 2015

+1

@tugberkugurlu

This comment has been minimized.

Show comment
Hide comment
@tugberkugurlu

tugberkugurlu Oct 27, 2015

Member

👍 change this, break me 😄 PowerShell also has Debug as the lowest level.

Member

tugberkugurlu commented Oct 27, 2015

👍 change this, break me 😄 PowerShell also has Debug as the lowest level.

@tugberkugurlu

This comment has been minimized.

Show comment
Hide comment
@tugberkugurlu

tugberkugurlu Oct 27, 2015

Member

sorry, I was thinking this is the serilog repo, got excited. I would say Debug is the correct lowest level. PowerShell also has Debug as the lowest level. So, 👎

Member

tugberkugurlu commented Oct 27, 2015

sorry, I was thinking this is the serilog repo, got excited. I would say Debug is the correct lowest level. PowerShell also has Debug as the lowest level. So, 👎

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Oct 27, 2015

Contributor

@tugberkugurlu just to clarify why I think this is a worthwhile change, I don't think it's so much a question of correctness, since "debugging" and "verbose logs" go hand in hand. For example:

The Write-Verbose cmdlet writes text to the verbose message stream in Windows PowerShell. Typically, the verbose message stream is used to deliver information about command processing that is used for debugging a command.

https://technet.microsoft.com/en-us/library/hh849951.aspx

It's probably one of those things where whichever you see first seems like the natural choice (as with many things you can find valid rationales for both).

My +1 was with respect to ease of integration with NLog and Serilog, which are going to appear in .NET apps alongside this library, and in suites of apps especially while large teams move onto ASP.NET 5.

My $0.02 of course :-) Cheers!

Contributor

nblumhardt commented Oct 27, 2015

@tugberkugurlu just to clarify why I think this is a worthwhile change, I don't think it's so much a question of correctness, since "debugging" and "verbose logs" go hand in hand. For example:

The Write-Verbose cmdlet writes text to the verbose message stream in Windows PowerShell. Typically, the verbose message stream is used to deliver information about command processing that is used for debugging a command.

https://technet.microsoft.com/en-us/library/hh849951.aspx

It's probably one of those things where whichever you see first seems like the natural choice (as with many things you can find valid rationales for both).

My +1 was with respect to ease of integration with NLog and Serilog, which are going to appear in .NET apps alongside this library, and in suites of apps especially while large teams move onto ASP.NET 5.

My $0.02 of course :-) Cheers!

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Oct 27, 2015

Contributor

@ ASPNET team - any feedback on this issue? Thoughts:

  1. We're going to change it (before RTM)
  2. We're too busy to think about this but we will answer this before RTM
  3. We're too busy to think about this and will deal with it POST RTM
  4. We're not going to change it because XXXX reason(s).

Cheers :)

Contributor

PureKrome commented Oct 27, 2015

@ ASPNET team - any feedback on this issue? Thoughts:

  1. We're going to change it (before RTM)
  2. We're too busy to think about this but we will answer this before RTM
  3. We're too busy to think about this and will deal with it POST RTM
  4. We're not going to change it because XXXX reason(s).

Cheers :)

@304NotModified

This comment has been minimized.

Show comment
Hide comment
@304NotModified

304NotModified Oct 27, 2015

Contributor

Good sum up @PureKrome!

Contributor

304NotModified commented Oct 27, 2015

Good sum up @PureKrome!

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Oct 28, 2015

Member

I think we're considering "merging" the Debug and Verbose values back into one. We had split them apart a while ago for some reasons that we don't think are needed anymore. If that's the case, we might be able to simplify things a bit.

Member

Eilon commented Oct 28, 2015

I think we're considering "merging" the Debug and Verbose values back into one. We had split them apart a while ago for some reasons that we don't think are needed anymore. If that's the case, we might be able to simplify things a bit.

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Oct 28, 2015

Contributor

Thanks @Eilon for the prompt update 👍 Very much appreciated 😃

Contributor

PureKrome commented Oct 28, 2015

Thanks @Eilon for the prompt update 👍 Very much appreciated 😃

@nblumhardt

This comment has been minimized.

Show comment
Hide comment
@nblumhardt

nblumhardt Oct 28, 2015

Contributor

+1 also, thanks @PureKrome and @Eilon! :-)

The merging idea sounds like a winner to me, FWIW.

Contributor

nblumhardt commented Oct 28, 2015

+1 also, thanks @PureKrome and @Eilon! :-)

The merging idea sounds like a winner to me, FWIW.

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Oct 28, 2015

Member

What @Eilon is failing to mention is that by merging we we're going to call it Verbug or Debose 😄

Member

davidfowl commented Oct 28, 2015

What @Eilon is failing to mention is that by merging we we're going to call it Verbug or Debose 😄

@PureKrome

This comment has been minimized.

Show comment
Hide comment
@PureKrome

PureKrome Oct 28, 2015

Contributor

lol

Contributor

PureKrome commented Oct 28, 2015

lol

@adamhathcock

This comment has been minimized.

Show comment
Hide comment
@adamhathcock

adamhathcock Oct 28, 2015

Contributor

+1 for Verbug

Contributor

adamhathcock commented Oct 28, 2015

+1 for Verbug

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Dec 11, 2015

Member

This has recently changed.

Member

muratg commented Dec 11, 2015

This has recently changed.

@muratg muratg closed this Dec 11, 2015

@304NotModified

This comment has been minimized.

Show comment
Hide comment
@304NotModified

304NotModified Dec 11, 2015

Contributor

Changed to what?

Contributor

304NotModified commented Dec 11, 2015

Changed to what?

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg
Member

muratg commented Dec 11, 2015

@muratg muratg modified the milestones: 1.0.0-rc2, 1.0.0 backlog Dec 11, 2015

@muratg muratg added 3 - Done and removed needs design labels Dec 11, 2015

@304NotModified

This comment has been minimized.

Show comment
Hide comment
@304NotModified

304NotModified Dec 11, 2015

Contributor

Thanks! Great news!

Contributor

304NotModified commented Dec 11, 2015

Thanks! Great news!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment