Logformatter null check #2200
Logformatter null check #2200
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2200 +/- ##
==========================================
+ Coverage 79.36% 80.96% +1.60%
==========================================
Files 217 217
Lines 6992 6994 +2
==========================================
+ Hits 5549 5663 +114
+ Misses 1443 1331 -112
|
| try | ||
| { | ||
| // Pass null as formatter function | ||
| this.logger.Log(LogLevel.Information, default, "Hello World!", null, null); |
There was a problem hiding this comment.
@jviau - Please review!
There was a problem hiding this comment.
I am not sure this was a bug to begin with. Many other log implementations all explicitly throw on a null formatter.
ConsoleLogger
EventLogLogger
ApplicationInsightsLogger
But with that said, the aggregate logger doesn't throw. So I am guessing it is up to each logger to decide what to do.
Either way, this change is fine within context of how otel currently behaves.
src/OpenTelemetry/CHANGELOG.md
Outdated
| @@ -5,6 +5,9 @@ | |||
| * Removes upper constraint for Microsoft.Extensions.Logging | |||
| dependencies. ([#2179](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2179)) | |||
|
|
|||
| * Fix bug which caused ILogger.Log calls to throw exception, when the | |||
There was a problem hiding this comment.
Good qn. After seeing some other implementations, which throw in Log(...formatter=null)., I am thinking we should follow the regular pattern of "not throwing from instrumentation APIs after successful initialization". So Otel logging provider can chose to not throw, end user may still see exception from other providers they have enabled.
And I can reword the changelog to just say:
"OpenTelemetry Logger modified to not throw when formatter is null"
ilogger.Log(LogLevel.Information, default, "Hello World!", null, null);(note the last param is null, which is the formatter) when combined withIncludeFormattedMessage=truewill result in an unhandled exception. This PR check if formatter is not null, before invoking it.Also removed some netcoreapp2.1 checks. (leftover cleanups)