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

Fix slf4j LinkageErrors for external plugins #2376

Merged
merged 9 commits into from
Jan 19, 2022

Conversation

eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Dec 29, 2021

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

@apmmachine
Copy link
Contributor

apmmachine commented Dec 29, 2021

💚 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
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
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
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
@AlexanderWert AlexanderWert added this to the 8.1 milestone Jan 17, 2022
Copy link
Member

@felixbarny felixbarny left a 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.

@eyalkoren
Copy link
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
5 participants