dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.48k stars 4.76k forks source link

Add LogLevel.Notice (for syslog) #39848

Open ygoe opened 4 years ago

ygoe commented 4 years ago

Background and Motivation

The LogLevel enum has a number of log levels for different logging events. Syslog, the Linux logging system that can also be used with Journald, has different (other/more/less) levels. While the highest syslog levels may only apply to system errors (out of scope of a .NET application), it has another interesting level: Notice. This is for messages that are not a warning but still more noteworthy than regular information messages (the default level for almost everything).

The Notice level can be used for anything that should stick out in the log, but is not a warning. It may include success messages or other important events that should not go unnoticed in masses of other log entries. Other log providers than Journal could also benefit from this new level in the same way.

While it would be possible to write Notice messages with the Journal class directly, it's not possible through the standard ILogger interface.

As LogLevel already has an entry that's not supported by Syslog, Trace, which is logged at the underlying Debug level, the fallback behaviour for loggers that cannot support Notice would be to use Information instead. Also, even if the log provider can't distinguish Notice, the LogLevel filter can be used to only write Notice or higher.

Proposed API

Umm, no code here. Just add LogLevel.Notice and sort it in with the other values between Information and Warning.

Alternative Designs

None. This is an ordered enum in a central position.

Risks

Inserting this new level between existing levels changes the numeric value of the higher levels Warning, Error and Critical. This is a breaking change and probably only possible in a new major version.

Dotnet-GitSync-Bot commented 4 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ygoe commented 4 years ago

@Dotnet-GitSync-Bot I've seen the label "area-Extensions-Logging" that might fit here. I'm not allowed to assign this.

alefranz commented 4 years ago

For completeness, here are Syslog 8 levels, however level 0 should not be used by applications so the only missing is Notice.

Level Description
0 Emergency: system is unusable
1 Alert: action must be taken immediately
2 Critical: critical conditions
3 Error: error conditions
4 Warning: warning conditions
5 Notice: normal but significant condition
6 Informational: informational messages
7 Debug: debug-level messages

from rfc5424 6.2.1 table 2

However, changing this enum will be a breaking change for all the logging libraries consuming it and unless these libraries have an interest to consume this extra values, I am not sure if it is worth doing a breaking change to add support for only a specific library using a specific target.

There will always be systems that have different levels available.

The 6 in use I believe have been historically the ones always used in .NET, probably because they match with the one used by Windows Event Log.

@maryamariyan, @tarekgh: I think this should be labelled area-Extensions-Logging

ghost commented 4 years ago

Tagging subscribers to this area: @maryamariyan See info in area-owners.md if you want to be subscribed.

ygoe commented 4 years ago

The Syslog Alert level is also missing, but I consider this only applicable to systems as well as Emergency.

to add support for only a specific library using a specific target.

What are those targets? I know three:

Most libraries I know write to text files of some kind, where adding another level (which maybe they already had but could never use through ILogger) is not a problem at all.

alefranz commented 4 years ago

The Syslog Alert level is also missing, but I consider this only applicable to systems as well as Emergency. Yes, sorry!

What are those targets? I know three:

There are many possible targets, for example here are the one supported by serilog: https://github.com/serilog/serilog/wiki/Provided-Sinks

Even when using files or databases, where adding a new field or possible value is not a problem per se, you generally have some solution on top of it to analyse or aggregate the logs that would have to take this new level into account (into in your queries or in your service to remap levels before emitting the logs), as even if you don't use it, libraries you rely on can start to adopt it once it is added.

Other targets have specific levels to map to, and not all have all the syslog levels.

ericstj commented 4 years ago

Just add LogLevel.Notice and sort it in with the other values between Information and Warning.

Today Information is 2 and Warning is 3. Enum values cannot change: it is severely breaking as usage captures the literal values in the calling code. It is possible to assign a new number and place it in the middle in metadata order, but presumably that would break comparison checks folks do on these levels. @davidfowl @maryamariyan any thoughts on growing LogLevel enum?

Moving to future for now as this is too late for 5.0.

maryamariyan commented 4 years ago

To summarize:

The ask is to add LogLevel.Notice and based on the syslog standard, the importance needs to lie between Information and Warning. So the drawback of adding LogLevel.Notice as @ericstj pointed to above is that we get breaking comparison checks like as seen in EventSourceLoggerProvider.

To fix this, there is no straightforward way but to have a breaking change. Keeping as a limitation for now.

ygoe commented 4 years ago

Maybe a second enum could be introduced that is accepted as an alternative and superset to LogLevel, with convenience conversion methods.