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 39 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 97.97571% with 10 lines in your changes missing coverage. Please review.

Project coverage is 86.67%. Comparing base (c45f1ad) to head (588c1ee).

Files with missing lines Patch % Lines
Snowflake.Data/Core/SFBlockingChunkDownloaderV3.cs 95.83% 3 Missing and 3 partials ⚠️
Snowflake.Data/Logger/EasyLoggerManager.cs 88.00% 0 Missing and 3 partials ⚠️
Snowflake.Data/Logger/SFLoggerImpl.cs 99.06% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1057      +/-   ##
==========================================
+ Coverage   86.17%   86.67%   +0.49%     
==========================================
  Files         131      134       +3     
  Lines       12549    12802     +253     
  Branches     1286     1317      +31     
==========================================
+ Hits        10814    11096     +282     
+ Misses       1416     1389      -27     
+ Partials      319      317       -2     

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

@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
Snowflake.Data/Logger/SFLoggerPair.cs Outdated Show resolved Hide resolved
Snowflake.Data/Client/SnowflakeDbCommand.cs Outdated Show resolved Hide resolved
{
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)

Snowflake.Data/Logger/SFLoggerImpl.cs Outdated Show resolved Hide resolved
{
message = SecretDetector.MaskSecrets(message).maskedText;
s_snowflakeLogger.Debug(message, ex);
s_customLogger.LogDebug(FormatBrackets(message), ex);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why FormatBrackets is executed only for Debug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was because only Debug has a log with brackets. It came from this query which led to this error

Edited to include FormatBrackets for other logging


private static SFLogger logger = null;
private static bool s_isSFLoggerEnabled = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't it be disabled by default? I thought Easy Logging feature would enable it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's correct

s_isSFLoggerEnabled is now set to false and the SF logger is disabled by default

}

public static void disableLogger()
public static void EnableCustomLogger()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's say that that at the beginning customer logger was disabled.
Let's say that some class has a static logger eq. private static readonly SFLogger logger = SFLoggerFactory.GetLogger<BasicAuthenticator>();
then the logger would have a value of new SFLoggerPair(..., ILoggerEmptyImpl();)
Let's say then the customer would call EnableCustomLogger().
The logger obtained in the first line would not be modified.
Am I right?
I think we need to reconfigure all the logger when EnableCustomLogger(), switching this field value is not enough.

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.

2 participants