-
Notifications
You must be signed in to change notification settings - Fork 114
fix(go/adbc/driver/snowflake): try to suppress stray logs #2608
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
Conversation
It appears gosnowflake uses logrus which defaults to "info" so that would explain why we're seeing the stray info log. |
Ah...the log message comes from a constructor for a global object, so it runs before we can suppress the message. While this PR is still good to have, it can't fix the underlying issue; we need Snowflake to fix it. |
The one thing I can do here is wire up the snowflake logger to the ADBC logger at least. |
Thanks for looking into this! A while back I patched gosnowflake specifically for a CRAN submission because it was creating files when initializing a global (maybe the same one). If useful, the patch is here: #1315 (comment) |
Ah, that's what I was vaguely remembering. That seems to be a different code path but I suppose the same line of thinking. |
Hmm, seems Snowflake has a global logger + it's pretty tied to the logrus interface; might not be good to bridge the two by default. Going to leave this PR as-is then |
FWIW Dewey, it seems the PR where Snowflake is fixing this is primarily adding some file-based cache of auth tokens...so we might just be trading one problem for another (if stray files are a concern for CRAN) |
Thanks for the heads up! We don't submit this one to CRAN and so it's fine, and even if we did, the problem is only if a file gets created when constructing a global (creating files during connection, while theoretically frowned upon by CRAN, is not something they would be able to detect because we very reasonably can't create a connection when running the tests on their servers). |
@zeroshade any comments here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expose setting the log level anywhere in the API?
Should we?
I looked at it, but the struggle is that we have a per-database log level and Snowflake has a global logger. Maybe we can config a separate logger per Snowflake connection though. But for now I figured we should just set this (even though it doesn't help) and wait for Snowflake to fix the underlying issue upstream. |
Looks like the snowflake PR merged, I will try and bump (temporarily) to see if it does fix the issue here |
It turns out gosnowflake also exposes a log level control; set this to "warn".
I also filed snowflakedb/gosnowflake#1332. We need the fix to be upstream since ultimately the log message is triggered by constructing a global, so we're powerless to suppress it. It was ultimately fixed by snowflakedb/gosnowflake#1327.