-
Notifications
You must be signed in to change notification settings - Fork 324
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
Fix slf4j LinkageErrors for external plugins #2376
Fix slf4j LinkageErrors for external plugins #2376
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
@jackshirazi just proposed that we get rid of both log4j and slf4j by taking only the bare minimum from whichever logging framework we choose and repackage it. It sounds like it could save us ton of trouble |
This may make it harder to use ecs-logging-java. If feasible, I'd like to keep us using the standard slf4j and log4j libraries. However, we can definitely think about stripping unused classes if possible. For example,
We may also just fail to load plugins that include classes that shouldn't be in there (slf4j, Byte Buddy, the Plugin SDK) by throwing an exception. This seems a bit safer as no loading them as there may be binary incompatibilities. But probably not a big issue.
Could we detect if the initiating class loader is the indy plugin CL and programmatically access the target CL? |
What if we just remove slf4j and use our own facade over log4j? If we add that through the SDK, external plugins can log into agent logs as well
Yes, I have this working. The problem is that for external plugin, the target CL is not necessarily any better, it could be just the instrumented library CL that probably doesn't have access to the logging library. |
apm-agent-core/src/main/java/co/elastic/apm/agent/bci/IndyBootstrap.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/TraceContext.java
Outdated
Show resolved
Hide resolved
...lication-server-integration-tests/src/test/java/co/elastic/apm/servlet/AbstractTomcatIT.java
Outdated
Show resolved
Hide resolved
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.
We should probably merge #2390 first.
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/bci/IndyBootstrap.java
Show resolved
Hide resolved
I added a verification mechanism to ensure that external plugins that contain agent dependencies are rejected with a proper log message. |
What does this PR do?
Fixes #2350
This PR touches several places related to external plugins:
ExternalPluginClassLoader
.ExternalPluginClassLoader
will still have visibility to slf4j, but those cannot effectively be loaded, so the plugin slf4j usages will use the agent slf4j.TraceContext
will just ignore attempts to set any type of agent CL as the initiating CL. This is enough to cause the usage of the context CL as a fallback, which will probably be sufficient in many cases. Support for setting the application class loader when starting a transaction via the public api #2369 contains a discussion with multiple proposals of how to improve that. Let's discuss this offline.Checklist