snowflakedb / snowflake-connector-net

Snowflake Connector for .NET
Apache License 2.0
175 stars 135 forks source link

SNOW-834781: Can we remove log4net and delegate logging to library consumers #204

Open rdagumampan opened 4 years ago

rdagumampan commented 4 years ago

Issue description

First thanks for working with this connector :)

I playing around with this connector and so far have been working nicely until I publish a self-contained .NET Core 3.1 CLI app. It throws up several errors and its largely due to dependencies to log4net.

C:\play\yuniql-snowflake\yuniql-platforms\snowflake\Yuniql.Snowflake.csproj : error NU1605: Detected package downgrade: System.Runtime.InteropServices from 4.3.0 to 4.1.0. Reference the package directly from the project to select a different version.  [C:\play\yuniql-snowflake\yuniql-cli\Yuniql.CLI.csproj]
C:\play\yuniql-snowflake\yuniql-platforms\snowflake\Yuniql.Snowflake.csproj : error NU1605:  Yuniql.Snowflake -> Snowflake.Data 1.1.2 -> log4net 2.0.8 -> System.Console 4.0.0 -> runtime.win.System.Console 4.3.0 -> System.Runtime.InteropServices (>= 4.3.0)  [C:\play\yuniql-snowflake\yuniql-cli\Yuniql.CLI.csproj]
C:\play\yuniql-snowflake\yuniql-platforms\snowflake\Yuniql.Snowflake.csproj : error NU1605:  Yuniql.Snowflake -> Snowflake.Data 1.1.2 -> log4net 2.0.8 -> System.Runtime.InteropServices (>= 4.1.0) [C:\play\yuniql-snowflake\yuniql-cli\Yuniql.CLI.csproj]
C:\p
...
...

From design point, I think its better if we delegate the responsibility of logging into the users of the library or let developers choose how to log by hooking to some event handlers or call backs. Right now, it looks like log4net has the heaviest baggage to carry on.

image

Configuration

.NET Core 3.0 CLI App Developing on Windows 10

rdagumampan commented 4 years ago

Removing several depedencies fixed several errors when used in .NET Core 3.+ self-contained app.

When Snowflake.Data is added to a .NET Core 3.0 library project and the project is referenced in a self-contained app (an .EXE app), the publishing fails. This is due to log4net carrying old dependencies causing downgrade detection of other dependencies.

So I forked the repo and made this change and all publish errors has been resolved :). This was my change.

For now, I am keeping a Snowflake.Data.Unofficial. Please let me know if this issue/change is in the roadmap.

Thanks, // @rdagumampan

marksmeltzer commented 4 years ago

I highly recommend removing log4net.

A simple strategy is this:

In addition to a singleton, app could also check app.config for logger configuration. This bit is less relevant for .NET Core. Moreover, if the user hasn't set a custom logger, the current functionality could be preserved "automagically" by scanning for the existence of the log4net driver assembly adjacent to the core assembly. If it finds the log4net driver assembly, load it using reflection and hook up the log4net logger class. That way, all an existing user would be required to do is to simply add a reference to the new package.

Moreover, the log4net assembly could even be included by default in the existing nuget package which with the reflection scenario would allow users to update without any breaking changes. However, customers that don't need/want the log4net driver could remove it from their project references and then implement their own custom logger by deriving from the base class and hooking it up via config / singleton / whatever.

agilenut commented 2 years ago

A simple strategy is this:

introduce some very simple log base class have a default console logger that implements the base class have a logging singleton, or some other simple mechanism, to allow consumers to specify a custom logger include an additional, optional log4net package that includes the current log code as log4net logger

This library should use Microsoft.Extensions.Logging to abstract the logging implementation like every other modern .net library. Don't invent your own. Using Microsoft.Extensions.Logging is the recommended way to allow consumers to choose which logging framework they want and avoid the unnecessary dependencies.

sfc-gh-dszmolka commented 1 year ago

hello and thank you all for your interest in this matter - also the suggestions are appreciated and welcome! also apologies for the long period without response. we'll take a look.

RalphCroft commented 4 months ago

Hi, how's the progress on this issue? It would be great if we could stop needing to add log4net to everything.

sfc-gh-dszmolka commented 4 months ago

hi - unfortunately i don't have any breakthroughs or closer estimations for implementation to share at this point, but will keep this thread updated once any new information becomes available.

If you're (or any of you interested in this change) are already a Snowflake customer - please do reach out to your Account Team and let them know you need this enhancement. This might help getting traction and prioritizing over other requests. Thank you in advance!