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

Rework how the SDK internal logging is output #3676

Merged
merged 14 commits into from
Feb 28, 2025
Merged

Conversation

normj
Copy link
Member

@normj normj commented Feb 27, 2025

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 package AWSSDK.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 the AWSSDK.Extensions.Logging.ILoggerAdaptor package and add the following code to their startup.

var builder = WebApplication.CreateBuilder(args);

var app = builder.Build();

app.Services.GetRequiredService<ILoggerFactory>()
    .ConfigureAWSSDKLogging();

For log4net since it uses static singleton the extensions pattern can't be done like Microsoft.Extensions.Logger. So a user would add the AWSSDK.Extensions.Logging.ILoggerAdaptor package and use code similar to the following:

[assembly: log4net.Config.XmlConfigurator(ConfigFile = "log4net.config")]

Log4NetAWSExtensions.ConfigureAWSSDKLogging();
var logger = LogManager.GetLogger(typeof(Program));

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 creating IAdaptorLogger 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

@normj normj added the v4 label Feb 27, 2025
@@ -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();
Copy link
Member Author

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.

Copy link
Contributor

@peterrsongg peterrsongg left a 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

@normj normj merged commit ac3e705 into v4-development Feb 28, 2025
1 check passed
@normj normj deleted the normj/v4-logging branch February 28, 2025 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants