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-625282: Make logrus dependency optional or drop it all together #616

Open
Thomasdezeeuw opened this issue Jul 11, 2022 · 5 comments
Open
Assignees
Labels
enhancement The issue is a request for improvement or a new feature status-triage_done Initial triage done, will be further handled by the driver team

Comments

@Thomasdezeeuw
Copy link

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?

@github-actions github-actions bot changed the title Make logrus dependency optional or drop it all together SNOW-625282: Make logrus dependency optional or drop it all together Jul 11, 2022
@Thomasdezeeuw
Copy link
Author

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

@sfc-gh-dszmolka
Copy link
Contributor

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 sfc-gh-dszmolka added the enhancement The issue is a request for improvement or a new feature label Mar 28, 2023
@bombsimon
Copy link
Contributor

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?

@sfc-gh-pfus
Copy link
Collaborator

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!

@bombsimon
Copy link
Contributor

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:

gosnowflake/log.go

Lines 49 to 60 in 5c89d42

func (log *defaultLogger) SetLogLevel(level string) error {
newEnabled := strings.ToUpper(level) != "OFF"
log.enabled = newEnabled
if newEnabled {
actualLevel, err := rlog.ParseLevel(level)
if err != nil {
return err
}
log.inner.SetLevel(actualLevel)
}
return nil
}

Since I seem to be able to get the global logger with GetLogger here

gosnowflake/log.go

Lines 413 to 415 in 5c89d42

func GetLogger() SFLogger {
return logger
}

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!

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage_done Initial triage done, will be further handled by the driver team label Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is a request for improvement or a new feature status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

6 participants