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: Can we remove log4net and delegate logging to library consumers #204

Open
rdagumampan opened this issue May 8, 2020 · 8 comments
Assignees
Labels
feature status-pr_pending_merge A PR is made and is under review status-triage_done Initial triage done, will be further handled by the driver team

Comments

@rdagumampan
Copy link

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

@github-actions github-actions bot changed the title Please remove log4net SNOW-159246: Please remove log4net May 8, 2020
@rdagumampan rdagumampan changed the title SNOW-159246: Please remove log4net Can we remove log4net and delegate logging to library consumers May 8, 2020
@rdagumampan
Copy link
Author

rdagumampan commented May 9, 2020

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.

  • Removed dependency to Log4net
  • Removed dependency to Common.Logging
  • Removed dependency to System.Configuration.ConfigurationManager
  • Removed SFConfigurationSectionHandler

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

Thanks, // @rdagumampan

@marksmeltzer
Copy link

I highly recommend removing log4net.

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

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
Copy link

agilenut commented Sep 1, 2022

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-igarish sfc-gh-igarish added question Issue is a usage/other question rather than a bug feature labels Mar 30, 2023
@sfc-gh-dszmolka sfc-gh-dszmolka added status-triage Issue is under initial triage and removed question Issue is a usage/other question rather than a bug labels Jun 7, 2023
@sfc-gh-dszmolka
Copy link
Contributor

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.

@sfc-gh-dszmolka sfc-gh-dszmolka added status-in_progress Issue is worked on by the driver team and removed status-triage Issue is under initial triage labels Jun 7, 2023
@sfc-gh-dszmolka sfc-gh-dszmolka added status-triage_done Initial triage done, will be further handled by the driver team and removed status-in_progress Issue is worked on by the driver team labels Feb 11, 2024
@RalphCroft
Copy link

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
Copy link
Contributor

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!

@sfc-gh-mhofman sfc-gh-mhofman changed the title Can we remove log4net and delegate logging to library consumers SNOW-834781: Can we remove log4net and delegate logging to library consumers Jul 17, 2024
@sfc-gh-dszmolka sfc-gh-dszmolka added the status-in_progress Issue is worked on by the driver team label Sep 26, 2024
@sfc-gh-dszmolka
Copy link
Contributor

short update: work on this item (of removing log4net) started, no ETA at this point, will keep this ticket posted.

@sfc-gh-dszmolka
Copy link
Contributor

Work is ongoing on #1057

@sfc-gh-dszmolka sfc-gh-dszmolka added status-pr_pending_merge A PR is made and is under review and removed status-in_progress Issue is worked on by the driver team labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature status-pr_pending_merge A PR is made and is under review status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests