Skip to content

Fix slf4j LinkageErrors for external plugins - #2376

Merged
eyalkoren merged 9 commits into
elastic:masterfrom
eyalkoren:external-plugin-slf4j-fix
Jan 19, 2022
Merged

Fix slf4j LinkageErrors for external plugins#2376
eyalkoren merged 9 commits into
elastic:masterfrom
eyalkoren:external-plugin-slf4j-fix

Conversation

@eyalkoren

@eyalkoren eyalkoren commented Dec 29, 2021

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #2350

This PR touches several places related to external plugins:

  • Class loading (@felixbarny please review this part)
    • The CL hierarchy for external plugin is currently not what we intended for the API classes. If we load the API classes by the IndyPluginCL, then the API plugin classes are loaded by another IndyPluginCL, one of which parents is the agent CL and the other is the external plugin IndyPluginCL. So I made sure those are loaded by the ExternalPluginClassLoader.
    • If slf4j classes are packaged into the plugin, I made sure to exclude them as well from the external plugin IndyPluginCL, because it is child-first, which means it will cause clashes with the slf4j that is available in the agent CL. It means that the ExternalPluginClassLoader will still have visibility to slf4j, but those cannot effectively be loaded, so the plugin slf4j usages will use the agent slf4j.
    • Our API plugin advice uses the instrumented class's (the API class) CL as the "initiating CL" (a term with a different meaning in the Java spec BTW 🙂 ). When the API is added to an application as a dependency, this makes perfect sense. But when it is loaded by an external plugin CL, this doesn't make sense. I changed it currently so that 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.
  • External plugin test apps (@jackshirazi please review this part)

Checklist

  • This is a bugfix

@ghost

ghost commented Dec 29, 2021

Copy link
Copy Markdown

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-19T13:55:18.024+0000

  • Duration: 46 min 56 sec

  • Commit: 8bd0b4f

Test stats 🧪

Test Results
Failed 0
Passed 2449
Skipped 16
Total 2465

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run compatibility tests : Run the JDK Compatibility test.

  • run integration tests : Run the APM-ITs.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@eyalkoren

Copy link
Copy Markdown
Contributor Author

@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

@felixbarny

Copy link
Copy Markdown
Member

@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, org.apache.logging.log4j.core.appender.<db|mom|nosql>.

If slf4j classes are packaged into the plugin

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.

Our API plugin advice uses the instrumented class's (the API class) CL as the "initiating CL"

Could we detect if the initiating class loader is the indy plugin CL and programmatically access the target CL?

@eyalkoren

Copy link
Copy Markdown
Contributor Author

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.

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

Could we detect if the initiating class loader is the indy plugin CL and programmatically access the target CL?

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.

@eyalkoren eyalkoren mentioned this pull request Jan 11, 2022
5 tasks
Comment thread apm-agent-core/src/main/java/co/elastic/apm/agent/bci/IndyBootstrap.java Outdated
@AlexanderWert AlexanderWert added this to the 8.1 milestone Jan 17, 2022

@felixbarny felixbarny left a comment

Copy link
Copy Markdown
Member

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.

@eyalkoren

Copy link
Copy Markdown
Contributor Author

I added a verification mechanism to ensure that external plugins that contain agent dependencies are rejected with a proper log message.
I also added a test to ensure that the dependencies list is kept up to date

@eyalkoren
eyalkoren requested a review from felixbarny January 19, 2022 12:03
@eyalkoren
eyalkoren enabled auto-merge (squash) January 19, 2022 13:55
@eyalkoren
eyalkoren merged commit e32985a into elastic:master Jan 19, 2022
@eyalkoren
eyalkoren deleted the external-plugin-slf4j-fix branch January 19, 2022 15:20
@eyalkoren eyalkoren mentioned this pull request Aug 23, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

slf4j MDC with java.lang.LinkageError, within the transaction created by external plugin and log correlation enabled

5 participants