Skip to content

Conversation

0xced
Copy link
Member

@0xced 0xced commented Mar 29, 2022

Before this commit:
LogLevel.None is converted to LogEventLevel.Fatal and events with a None level are logged as fatal.

After this commit:
LogLevel.None is observed and logs and events with a None level are ignored.

The None level is documented as such:

Not used for writing log messages. Specifies that a logging category should not write any messages.

Note: this erroneous behaviour was seen in a real-world scenario:

  1. BuildOrgConnectUri CoreClass () is logged as TraceEventType.Start
  2. TraceEventType.Start is converted to LogLevel.None
  3. LogLevel.None is converted to LogEventLevel.Fatal

As a result, BuildOrgConnectUri CoreClass () is logged as fatal whereas it should have been ignored.

Before this commit: `LogLevel.None` is converted to `LogEventLevel.Fatal` and events with a `None` level are logged as fatal. After this commit: `LogLevel.None` is observed and logs and events with a `None` level are ignored. The `None` level is documented as such: > Not used for writing log messages. Specifies that a logging category should not write any messages. Note: this erroneous behaviour was seen in a real-world scenario: 1. `BuildOrgConnectUri CoreClass ()` is [logged as `TraceEventType.Start `][1] 2. `TraceEventType.Start` is [converted to `LogLevel.None`][2] 3. `LogLevel.None ` is [converted to `LogEventLevel.Fatal`][3] As a result, `BuildOrgConnectUri CoreClass ()` is logged as fatal whereas it should have been ignored. [1]: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/blob/0.6.1/src/GeneralTools/DataverseClient/Client/ConnectionService.cs#L3207 [2]: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/blob/0.6.1/src/GeneralTools/DataverseClient/Client/DataverseTraceLogger.cs#L675 [3]: https://github.com/serilog/serilog-extensions-logging/blob/dev/src/Serilog.Extensions.Logging/Extensions/Logging/LevelConvert.cs#L39
@nblumhardt
Copy link
Member

Thanks for the PR!

To me, it seems like the behavior in linked snippet (2) is a bug (None shouldn't be used for log events, as you've quoted), but since this gets us closer to the behavior of the built-in logger I think it's a plus and we should merge 👍

@nblumhardt nblumhardt merged commit 7141ae2 into serilog:dev Apr 7, 2022
@0xced 0xced deleted the LogLevel-None branch April 8, 2022 07:56
@nblumhardt nblumhardt mentioned this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants