TraceSourceLogger now takes exception into account(adds it to the log…

[rizi]

TraceSourceLogger now takes the exception into account even if the formatter is not null, therefore the exception will be added to the log message.

This pull request fixes#42341

Please let me know if there is a better way to link the PR to the issue.

Br

[ghost]

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

[KalleOlaviNiemitalo]

It looks like this appends the exception to the message twice if formatter == null. Once would be enough.

The string.Format and CultureInfo.InvariantCulture references may be unnecessary; string concatenation could suffice. EventLogLogger does not use those when it appends an Exception to a StringBuilder. Exception classes typically do not implement IFormattable and thus cannot receive an IFormatProvider as a parameter; they just use the ambient cultures. On the other hand, exceptions should be rare, and exception-logging code might then not be performance-sensitive.

Should perhaps add a test in TraceSourceLoggerProviderTest.cs, to verify that Exception.Message occurs in the trace message exactly once. That file has #if NETFRAMEWORK though, so I am not sure the test class is even compiled for .NET 5.

[dnfadmin]

All CLA requirements met.

[rizi]

It looks like this appends the exception to the message twice if formatter == null. Once would be enough.

The string.Format and CultureInfo.InvariantCulture references may be unnecessary; string concatenation could suffice. EventLogLogger does not use those when it appends an Exception to a StringBuilder. Exception classes typically do not implement IFormattable and thus cannot receive an IFormatProvider as a parameter; they just use the ambient cultures. On the other hand, exceptions should be rare, and exception-logging code might then not be performance-sensitive.

Should perhaps add a test in TraceSourceLoggerProviderTest.cs, to verify that Exception.Message occurs in the trace message exactly once. That file has #if NETFRAMEWORK though, so I am not sure the test class is even compiled for .NET 5.

@KalleOlaviNiemitalo I fixed the logical issue, that the exception would have been added twice to the log message when the formatter is null.
I also removed the "string formatting".

Regarding tests:
I already asked in the corresponding issue where tests for the LogProvider can be found, I think it would not make sense to add a test for the TraceSourceLogProvider, instead there should already be tests for the TraceSourceLogger, and if there are tests I will of course add one (but I can't find the tests/I'm not sure if there any at all).

[KalleOlaviNiemitalo]

@rizi, there is TraceSourceLoggerTest.cs in the same directory. TraceSourceLoggerProviderTest.BufferedConsoleTraceListener would be useful for the new test, though.

[rizi]

@rizi, there is TraceSourceLoggerTest.cs in the same directory. TraceSourceLoggerProviderTest.BufferedConsoleTraceListener would be useful for the new test, though.

Thx for clarifying, I will add tests in the TraceSourceLoggerTest.cs file.

Can someone please help me to compile the Microsoft.Extensions.Logging.sln

Here is what I did so far:

• build.cmd clr+libs -rc Release (finished with 0 warnings and 0 errors)
• build.cmd -vs Microsoft.Extensions.Logging

As soon as VS 2019 (16.8 preview 3) opens I can't compile the solution or run tests:
Error:
1>------ Build started: Project: TestUtilities, Configuration: Debug Any CPU ------
Error occurred while restoring NuGet packages: Invalid restore input. Duplicate frameworks found: 'net5.0, net461, net5.0'. Input files: C:\Dev\Git\runtime\src\libraries\Microsoft.Extensions.Logging\tests\Common\Microsoft.Extensions.Logging.Tests.csproj.
1>CSC : error CS0006: Metadata file 'C:\Dev\Git\runtime\artifacts\bin\System.Runtime.CompilerServices.Unsafe\net45-Debug\System.Runtime.CompilerServices.Unsafe.dll' could not be found
1>Done building project "TestUtilities.csproj" -- FAILED.
1>TestUtilities -> C:\Dev\Git\runtime\artifacts\bin\TestUtilities\net5.0-Debug\TestUtilities.dll
2>------ Build started: Project: Microsoft.Extensions.Logging.Tests, Configuration: Debug Any CPU ------

It seems that VS(nuget restore) are confused with $(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent).

As there no way make Visual Studio work?

[rizi]

@rizi, there is TraceSourceLoggerTest.cs in the same directory. TraceSourceLoggerProviderTest.BufferedConsoleTraceListener would be useful for the new test, though.

Thx for clarifying, I will add tests in the TraceSourceLoggerTest.cs file.

Can someone please help me to compile the Microsoft.Extensions.Logging.sln

Here is what I did so far:

• build.cmd clr+libs -rc Release (finished with 0 warnings and 0 errors)
• build.cmd -vs Microsoft.Extensions.Logging

As soon as VS 2019 (16.8 preview 3) opens I can't compile the solution or run tests:
Error:
1>------ Build started: Project: TestUtilities, Configuration: Debug Any CPU ------
Error occurred while restoring NuGet packages: Invalid restore input. Duplicate frameworks found: 'net5.0, net461, net5.0'. Input files: C:\Dev\Git\runtime\src\libraries\Microsoft.Extensions.Logging\tests\Common\Microsoft.Extensions.Logging.Tests.csproj.
1>CSC : error CS0006: Metadata file 'C:\Dev\Git\runtime\artifacts\bin\System.Runtime.CompilerServices.Unsafe\net45-Debug\System.Runtime.CompilerServices.Unsafe.dll' could not be found
1>Done building project "TestUtilities.csproj" -- FAILED.
1>TestUtilities -> C:\Dev\Git\runtime\artifacts\bin\TestUtilities\net5.0-Debug\TestUtilities.dll
2>------ Build started: Project: Microsoft.Extensions.Logging.Tests, Configuration: Debug Any CPU ------

It seems that VS(nuget restore) are confused with $(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent).

As there no way make Visual Studio work?

TraceSourceLogger is an internal class, therefore I can't create an instance with new TraceSourceLogger(...);
Using

 var factory = TestLoggerBuilder.Create(builder => builder.AddTraceSource(testSwitch));
var logger = factory.CreateLogger("Test");

[KalleOlaviNiemitalo]

@rizi, Microsoft.Extensions.Caching.Memory has InternalsVisibleToAttribute to let tests access the internals. I guess you can do the same in Microsoft.Extensions.Logging.TraceSource if you need to. Perhaps that would require SkipUseReferenceAssembly (described in #35606 (comment)) as well, because the reference assembly would not have the InternalsVisibleToAttribute.

[rizi]

@rizi, Microsoft.Extensions.Caching.Memory has InternalsVisibleToAttribute to let tests access the internals. I guess you can do the same in Microsoft.Extensions.Logging.TraceSource if you need to. Perhaps that would require SkipUseReferenceAssembly (described in #35606 (comment)) as well, because the reference assembly would not have the InternalsVisibleToAttribute.

Thx for the info, I wasn't sure if that's ok.
Is a mock framework (moq, fakeiteasy,..) for the dotnet/runtime in use? Or how do you assert calls that are made against e.q an interface for a object/class under test?

Any idea how I can make the solution work in visual studio, it's really cumbersome to work with vs code and the command line if you are used to have a feature rich ide.

Br

[KalleOlaviNiemitalo]

To be clear, I don't have any authority in this project. But if it is OK to use InternalsVisibleToAttribute for tests in Microsoft.Extensions.Caching.Memory, I think it will be OK to use that elsewhere as well.

[KalleOlaviNiemitalo]

Is a mock framework (moq, fakeiteasy,..) for the dotnet/runtime in use?

The Microsoft.Extensions.Logging.Tests project uses Moq:

runtime/src/libraries/Microsoft.Extensions.Logging/tests/Common/Microsoft.Extensions.Logging.Tests.csproj

Line 24 in f4a7e3d

<PackageReferenceInclude="Moq"Version="$(MoqVersion)" />

runtime/src/libraries/Microsoft.Extensions.Logging/tests/Common/LoggerFactoryExtensionsTest.cs

Lines 19 to 22 in f4a7e3d

	varfactory=newMock<ILoggerFactory>();
	factory.Setup(f =>f.CreateLogger(
	It.IsAny<string>()))
	.Returns(newMock<ILogger>().Object);

[tarekgh]

I think for testing, we can just use the public APIs which will exercise this internal code and get the expected results. I am not sure why we need to test the internal classes? if it is really necessary, I would say use reflection to do so.

[maryamariyan]

Thanks for the PR @rizi. I have some nit comments and recommendations on exception delimiter portion of code.

[maryamariyan]

Aside from nit comments, LGTM.

Thanks @rizi

[maryamariyan]

Just applied two nit commits on your PR. (remove extra new lines or whitespaces) and merging.

Thanks @rizi for your PR submission.