-
Notifications
You must be signed in to change notification settings - Fork 863
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
Rework how the SDK internal logging is output #3676
Conversation
@@ -23,6 +23,5 @@ public interface ILogger | |||
void Debug(Exception exception, string messageFormat, params object[] args); | |||
void DebugFormat(string messageFormat, params object[] args); | |||
void Error(Exception exception, string messageFormat, params object[] args); | |||
void Flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flush is not supported in Microsoft.Extensions.Logger and we only used in 2 places in the entire SDK that I don't think needed to be used there.
...nsions.Logging.Log4NetAdaptor/AWSSDK.Extensions.Logging.Log4NetAdaptor.4.0.0-preview.8.nupkg
Outdated
Show resolved
Hide resolved
...src/AWSSDK.Extensions.Logging.Log4NetAdaptor/AWSSDK.Extensions.Logging.Log4NetAdaptor.csproj
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested in a web app, approved with some minor non-blocking comments
Description
In V3 the logging is output to either the console or if Log4net if found via reflection it attaches itself to that. The reflection approach was done to avoid the SDK taking a dependency on log4net. This was also done back in the V2 days when the SDK was a single .NET assembly and we hadn't ventured into creating multiple packages yet.
Log4net is likely not the most popular logging framework these days. We know Microsoft.Extensions.Logging is an ask but unlike log4net it doesn't rely on a singleton programming model and we need an actual instance if
ILogFactory
to do logging. But again we don't want AWSSDK.Core taking dependency on all possible logging frameworks.This redesign removes the reflection for log4net and instead moves it out into a separate package
AWSSDK.Extensions.Logging.Log4NetAdaptor
along with another packageAWSSDK.Extensions.Logging.ILoggerAdaptor
for Microsoft.Extensions.Logger support. Console and System.Diagnostics are still built into AWSSDK.Core since they don't require any dependencies.If a user wants integrate the SDK logging with
Microsoft.Extensions.Logger
they would add theAWSSDK.Extensions.Logging.ILoggerAdaptor
package and add the following code to their startup.For log4net since it uses static singleton the extensions pattern can't be done like
Microsoft.Extensions.Logger
. So a user would add theAWSSDK.Extensions.Logging.ILoggerAdaptor
package and use code similar to the following:How the SDK internally logs has not changed. I did do some adjustments to SDK log levels due to what I considered too chatting logging messages. Basically downgraded log messages saying there was not value for something to debug level.
The internals of the SDK's internal logger have changed by removing all the reflection for log4net. The internal logger now keeps a collection of registered
IAdaptorLoggerFactory
that are implemented per supported logging framework. The factory is in charge of creatingIAdaptorLogger
that are wrappers around the loggers for the logging framework.This PR does add 2 new extension NuGet packages and the build system will need to be updated to package them up.
Motivation and Context
Remove legacy reflection that causes Native AOT issues and might cause unexpected logging by users that just want to turn log4net in their own applications.
Testing
Dry run: DRY_RUN-8947d6cb-2e55-4539-a792-e52de2753086 successful