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

SNOW-834781: Remove log4net and delegate logging to consumers #1057

Open
wants to merge 80 commits into
base: master
Choose a base branch
from

Conversation

sfc-gh-ext-simba-lf
Copy link
Collaborator

Description

Remove the log4net dependency and replace with the Microsoft logging interface so users can plug in their own custom loggers

Checklist

  • Code compiles correctly
  • Code is formatted according to Coding Conventions
  • Created tests which fail without the change (if possible)
  • All tests passing (dotnet test)
  • Extended the README / documentation, if necessary
  • Provide JIRA issue id (if possible) or GitHub issue id in PR name

…nector-net into SNOW-834781-Remove-log4net

# Conflicts:
#	Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs
#	Snowflake.Data.Tests/UnitTests/Configuration/EasyLoggingConfigFinderTest.cs
#	Snowflake.Data/Core/ArrowResultSet.cs
#	Snowflake.Data/Core/FileTransfer/StorageClient/SFGCSClient.cs
#	Snowflake.Data/Core/HttpUtil.cs
#	Snowflake.Data/Core/SFBlockingChunkDownloaderV3.cs
#	Snowflake.Data/Core/SFStatement.cs
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 94.22633% with 25 lines in your changes missing coverage. Please review.

Project coverage is 86.62%. Comparing base (bb4f834) to head (927c59f).

Files with missing lines Patch % Lines
Snowflake.Data/Logger/SFLoggerFactory.cs 74.19% 7 Missing and 1 partial ⚠️
Snowflake.Data/Logger/SFLoggerPair.cs 84.61% 8 Missing ⚠️
Snowflake.Data/Core/SFBlockingChunkDownloaderV3.cs 95.83% 3 Missing and 3 partials ⚠️
Snowflake.Data/Logger/SFConsoleAppender.cs 90.00% 1 Missing ⚠️
Snowflake.Data/Logger/SFLoggerImpl.cs 98.75% 0 Missing and 1 partial ⚠️
Snowflake.Data/Logger/SFRollingFileAppender.cs 98.18% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1057      +/-   ##
==========================================
+ Coverage   86.24%   86.62%   +0.37%     
==========================================
  Files         132      137       +5     
  Lines       12672    12844     +172     
  Branches     1300     1320      +20     
==========================================
+ Hits        10929    11126     +197     
+ Misses       1417     1397      -20     
+ Partials      326      321       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf marked this pull request as ready for review November 13, 2024 19:23
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf requested a review from a team as a code owner November 13, 2024 19:23
{
ILoggerFactory factory = LoggerFactory.Create(
builder => builder
.AddConsole()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it mean that we create a custom logger with logging to console always enabled?
Could we defer the decision about appenders to the customer configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means if a customer enables custom logging without setting their own logger then a console logger will be created

If a customer sets the logger then it will return their custom logger with their chosen appenders (logging to console is not enabled unless they added it themselves)

var repository = (log4net.Repository.Hierarchy.Hierarchy)LogManager.GetRepository();
var rootLogger = (log4net.Repository.Hierarchy.Logger)repository.GetLogger("Snowflake.Data");
rootLogger.Level = log4netLevel;
var rootLogger = SFLogRepository.GetRootLogger();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've lost. Could you explain the relation between RootLogger and SFLoggerPair.s_snowflakeLogger?
Why do we need root logger?

Copy link
Collaborator Author

@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SFLoggerPair.s_snowflakeLogger contains the type/class where the logging is being created (SnowflakeDbConnection, SnowflakeDbDataReader, etc.)

The rootLogger contains the log level and appenders set from ReconfigureEasyLogging or ResetEasyLogging

If there's no rootLogger, then calling ReconfigureEasyLogging would not affect the log level and appenders of SFLoggerPair.s_snowflakeLogger

Since the original log4net used a rootLogger, I tried to minimize any changes by making SFlogger similar

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored it so the log level/appender are static and root logger is no longer necessary. But if you prefer the original way that log4net used to do it let me know

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to understand the big picture.

  1. How the user can register his logger implementation?
  • I see that he needs to set SFLoggerFactory.s_customLogger by SnowflakeDbLoggerFactory.SetCustomLogger()
  • so the consequence is that the user sets for all drivers's classes the same implementation - is it good?
  • I know that for instance in log4net you could define different loggers for different classes
  • Is there a way to set different customer loggers for different classes?
  • Is there a generic way to register ILogger implementation?
  1. How does Snowflake logger work?
  • I see that SFLoggerPair has a field of _snowflakeLogger
  • Once the SFLoggerPair is created, I don't see the way to change the snowflakeLogger
  • I see that EasyLogging changes appenders for SFLoggerImpl
  • but how Easy Logging would enable logging if the classes would have SFLoggerEmptyImpl implementation in their SFLoggerPair?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that he needs to set SFLoggerFactory.s_customLogger by SnowflakeDbLoggerFactory.SetCustomLogger()

  • That's correct

so the consequence is that the user sets for all drivers's classes the same implementation - is it good?

  • Yes, it sets the same implementation for all classes

I know that for instance in log4net you could define different loggers for different classes

  • Yes, when log4net was used directly by the connector it defined different loggers for different classes using SFLoggerFactory.GetLogger<Class>(). Now the connector does a similar thing for SFLogger

Is there a way to set different customer loggers for different classes?

  • There probably is, maybe a function that adds a custom logger per class. The connector could check a dictionary of custom loggers for that specific class and if not, use the custom logger from SetCustomLogger

Is there a generic way to register ILogger implementation?

I see that SFLoggerPair has a field of _snowflakeLogger
Once the SFLoggerPair is created, I don't see the way to change the snowflakeLogger

  • It's changed through the appender

I see that EasyLogging changes appenders for SFLoggerImpl

  • That's correct

but how Easy Logging would enable logging if the classes would have SFLoggerEmptyImpl implementation in their SFLoggerPair?

  • The SFLoggerEmptyImpl was done for test purposes and can only be enabled internally. The SFLoggerImpl should be what SFLoggerPair always uses

{
internal class SFLoggerPair : SFLogger
{
private readonly SFLogger s_snowflakeLogger;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not static but has prefix s_

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed

…nector-net into SNOW-834781-Remove-log4net

# Conflicts:
#	Snowflake.Data.Tests/UnitTests/Logger/SFLoggerTest.cs
#	Snowflake.Data/Configuration/SFConfigurationSectionHandler.cs
#	Snowflake.Data/Core/SFBlockingChunkDownloaderV3.cs
#	Snowflake.Data/Logger/Log4netImpl.cs
#	Snowflake.Data/Logger/SFLogger.cs
#	Snowflake.Data/Logger/SFLoggerEmptyImpl.cs
#	Snowflake.Data/Logger/SFLoggerFactory.cs
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.

4 participants