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

Observe Microsoft.Extensions.Logging.LogLevel.None #203

Merged
merged 1 commit into from
Apr 7, 2022

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants