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-1229142: Should not shade apache commons logging #1667

Open
OskarKjellin opened this issue Mar 11, 2024 · 10 comments
Open

SNOW-1229142: Should not shade apache commons logging #1667

OskarKjellin opened this issue Mar 11, 2024 · 10 comments
Assignees
Labels
bug status-triage_done Initial triage done, will be further handled by the driver team

Comments

@OskarKjellin
Copy link

Because the apache http client is shaded, the logging it uses (apache commons logging) also becomes shaded.
When this is shaded it's not possible to use the commons logging to slf4j bridge, meaning that our logs get filled with:

And we don't get any logs from apache http client.```

commons-logging:commons-logging should probably be excluded?

1. What version of JDBC driver are you using?
3.15.0
  
2. What did you do?
Used the snowflake ingest java client

3. What did you expect to see?

No warning logs and slf4j bridge working
@github-actions github-actions bot changed the title Should not shade apache commons logging SNOW-1229142: Should not shade apache commons logging Mar 11, 2024
@sfc-gh-sghosh sfc-gh-sghosh self-assigned this Mar 14, 2024
@sfc-gh-sghosh sfc-gh-sghosh added status-triage Issue is under initial triage and removed bug labels Mar 14, 2024
@sfc-gh-sghosh
Copy link
Contributor

Hello @OskarKjellin ,

Thanks for raising the issue, could you provide example (sample jdbc application or code snippet) with jdbc logs how Snowflake jdbc connector is shadding logs from the Apache HTTP client.

Regards,
Sujan

@OskarKjellin
Copy link
Author

can't really share any code snippet unfortunately, we're running things through the java ingest sdk.
When we run our code we see in our logs
log4j:WARN No appenders could be found for logger (net.snowflake.client.jdbc.internal.apache.http.client.protocol.RequestAddCookies).

And if you check the source for that class you can clearly see:
import net.snowflake.client.jdbc.internal.apache.commons.logging.Log

So it's shading apache commons logging, meaning that you cannot bridge it to slf4j

@sfc-gh-sghosh
Copy link
Contributor

Hello @OskarKjellin ,

Could you please use or switch to thin jar and please add jcl-over-slf4j and exclude commons-logging in application pom.xml and try.

Regards,
Sujan

@OskarKjellin
Copy link
Author

Hi again,

We're getting the dependency transitively through snowflake-ingest-java, and from what I can tell from this thread I've had with them it seems like it wouldn't matter which one I pull in ? snowflakedb/snowflake-ingest-java#714

And I would still like to use the shaded version if possible to avoid possible dependency collision problems.

Is your take that it's expected behaviour to shade the logging facade?

@sfc-gh-sghosh sfc-gh-sghosh removed their assignment Mar 25, 2024
@sfc-gh-sghosh sfc-gh-sghosh added bug status-triage_done Initial triage done, will be further handled by the driver team and removed status-triage Issue is under initial triage labels Mar 25, 2024
@sfc-gh-sghosh
Copy link
Contributor

Hello @OskarKjellin ,

we confirmed this gap and will work on eliminating it. Will keep this thread posted.

Regards,
Sujan

@OskarKjellin
Copy link
Author

Awesome, happy to hear! Thanks for confirming.

@sfc-gh-wfateem
Copy link
Collaborator

@OskarKjellin I'm trying to figure out what might be happening in your scenario with this warning that you're seeing:
log4j:WARN No appenders could be found for logger (net.snowflake.client.jdbc.internal.apache.http.client.protocol.RequestAddCookies)

In theory, I should see the same thing if I try to run an application that uses the Ingest Java SDK library, correct? What version of the ingest SDK were you using?

Is the following Maven coordinates the only Snowflake dependency defined in your build tool's configuration?
net.snowflake:snowflake-ingest-sdk:2.1.0

Can you also list for me the log4j and slf4j libraries you're using, please?

@OskarKjellin
Copy link
Author

@sfc-gh-wfateem I think part of the reason might be that there is some other log4j in the classpath.
But ignoring that specific problem, IMO the actual problem is that logging frameworks should never be shaded.

@sfc-gh-wfateem
Copy link
Collaborator

@OskarKjellin I agree. I'm not sure what the background was leading to that decision, assuming it wasn't done by mistake.

@OskarKjellin
Copy link
Author

@sfc-gh-wfateem any update on getting this fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

4 participants