gosnowflake icon indicating copy to clipboard operation
gosnowflake copied to clipboard

SNOW-625282: Make logrus dependency optional or drop it all together

Open Thomasdezeeuw opened this issue 2 years ago • 5 comments

The dependency on logrus creates two problems:

  1. I'm currently getting a lot of logs in a different format than my log parser is expected by using Snowflake.
  2. Creating a customer logging implementation that doesn't use logrus is basically not possible.

Would it be possible to make the logrus dependency optional or maybe remove it all together?

Thomasdezeeuw avatar Jul 11 '22 10:07 Thomasdezeeuw

Any feedback on this? If accept I could start a pr.

Thomasdezeeuw avatar Jul 21 '22 15:07 Thomasdezeeuw

hi, thank you for submitting this request with us. we'll take a look - as with other enhancements, can't promise any timeline on the ETTR so thank you for bearing with us ! a PR of course would be very helpful to get things moving ; however i'm not sure if simply removing logrus would be feasible here or more like a breaking change.

sfc-gh-dszmolka avatar Mar 28 '23 13:03 sfc-gh-dszmolka

I would also be interested in dropping logging. I don't think it's very common to do arbitrary logging from libraries like this. I think removing the logging and propagating errors to the caller would make more sense. If it's not an error I don't think it needs to be logged.

If however you still want to use logging from this codebase, maybe considering something more flexible like logr which has an extensive list of implementations or create your own slimmed down interface uses can implement themselves (info- and error severity should be enough).

And if that also isn't of interest, maybe you could expose the global logger here so users can overwrite it?

bombsimon avatar Aug 17 '23 09:08 bombsimon

Hi all! We started considering this issue!

I have a strong Java background and in Java we have a wonderful SLF4J library which is de facto a standard for Java logging API. SLF4J is not a logger itself, it is more like an API which is implemented by multiple logging libraries like logback or log4j. Still, this is great, because all libraries use only SLF API and not come with logging backend of their choice. Not sure if this is it, but it seems that in Go similar library is https://github.com/go-logr/logr

Another solution might be to use https://go.dev/blog/slog . Unfortunately, this is brand new thing, which was introduced in Go 1.21. We have to support older go versions for a few couple of quarters (fingers crossed that less than more).

If you are interested in such changes, please add a reaction. For those in favour in going to logr - react with 🚀 For those in favour in going to slog (will take more time) - react with 🎉 For those in favour of something else - please add your suggestions.

Thanks in advance!

sfc-gh-pfus avatar Jan 24 '24 13:01 sfc-gh-pfus

Thanks for picking this up!

As I wrote in my previous post I'd rather not have logs at all and I'm not sure any of logr or slog solves that. Moving to logr would allow me to build my own noop-logger or maybe use something like buflogr but for me I think best would be to expose the enable field on the defaultLogger.

And browsing the code now again I see that you seem to have some magic word where setting the level to OFF would disable the logger that I missed last time: https://github.com/snowflakedb/gosnowflake/blob/5c89d42a335ea41256e87ff71317ef6f8df6a086/log.go#L49-L60

Since I seem to be able to get the global logger with GetLogger here https://github.com/snowflakedb/gosnowflake/blob/5c89d42a335ea41256e87ff71317ef6f8df6a086/log.go#L413-L415

I guess I should be able to turn it off completely with:

gosnowflake.GetLogger().SetLogLevel("OFF")

Maybe it would be worth documenting that and/or using that example in cmd/logger/logger.go or add an explicit Disable method on the logger?


So with my issue solved, I guess that the most flexible way to consider most users log setup and also the one you wrote would be fastest, moving to logr might be a better bet than slog as well!

bombsimon avatar Jan 24 '24 22:01 bombsimon