Fix classloader for cross-plugin dependencies

[SylvainJuge]

What does this PR do?

Fixes cross-plugin dependencies class-loading issues.
Fixing the issue is quite trivial, but the real challenge is creating a test to prevent similar issues.

Initially reported in the forum.

• minor-refactor: move 'http.client' to 'httpclient' for consistency
• Implement test to prevent similar issue (parsing plugin dependencies)
• add CL root package customizer for all inter-plugin dependencies

Checklist

• This is a bugfix
  • I have updated CHANGELOG.asciidoc
  • I have added tests that would fail without this fix
  • Fixing/removing one exclusion for the log-shading plugin, won't be necessary once Reimplementing log correlation #2428 is merged.

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-03-14T14:24:36.748+0000

• Duration: 45 min 26 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2745
Skipped	16
Total	2761

💚 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 tests.

• run jdk compatibility tests : Run the JDK Compatibility tests.

• run integration tests : Run the Agent Integration tests.

• run end-to-end tests : Run the APM-ITs.

• run windows tests : Build & tests on windows.

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

[felixbarny]

All plugins, except for the spring mvc one, don't have a PluginClassLoaderRootPackageCustomizer on purpose. The modules they depend on, such as apm-httpclient-core, apm-cassandra-core-plugin, and apm-redis-common don't depend on any specific library classes. Therefore, it's better to load them from the agent class loader so that the plugin class loaders don't contain redundant classes that increase the memory impact.

However, this makes it hard to have a heuristic and create an automated test when to or when not to require a PluginClassLoaderRootPackageCustomizer`.

[SylvainJuge]

I agree with you @felixbarny , we should aim to minimize extra memory usage when we can, in this case I think the benefits/cost seems worth it:

• We can assume that loading multiple times those classes is quite rare, for example using multiple redis drivers at the same time is not that common. Also the number of classes and their size is usually small.
• when the test fails there is a clear indication of what to do, thus it's a huge gain for us as detecting this with integration tests would be too tedious as it would require to cover every plugin.

Maybe we could re-think this later with a better approach to provide cross-plugin visibility without duplication of loaded classes if we find that this memory overhead is becoming an issue.

[felixbarny]

Good points.

Just a question on the feasibility of making the heuristic a bit smarter: Would it be possible to detect that HttpClientHelper is from the apm-httpclient-core plugin which doesn't have any dependencies, and therefore it's not required to add a PluginClassLoaderRootPackageCustomizer?

[SylvainJuge]

Good idea, I have added this case in last commit (603596c) so it simplifies things a lot for HTTP client plugins.

[felixbarny]

Nice 👏

You can also remove the apm-jdbc-plugin dependency in the apm-cassandra-core-plugin as it is not needed. I forgot to remove it after I have moved some parts out of the JDBC plugin into the core.

As the apm-redis-common module also doesn't have any dependencies, you should be able to remove the related PluginClassLoaderRootPackageCustomizer that add the co.elastic.apm.agent.redis package.

[SylvainJuge]

[for reviewer] the try-catch block is similar in the Servlet adapter, thus removing duplication.

[github-actions]

/test

[felixbarny]

Excellent!