elastic / elastic/apm-agent-java

Improve classloader structure and isolation for external plugins.

Open
#3,156 0 comments 0 reactions 0 assignees View on GitHub
agent-java technical-debt
Dominant language
Java
Stars
594
Forks
338
Avg merge
1d 13h
Merged PRs (30d)
25

Description

The current classloading structure is quite complex when it comes to external plugin.
This can make things difficult when adding dependencies within our agent which might interfere with external plugins and makes it easy to introduce bugs such as #3145 .

This issue proposes to restructure the classloading a little (not changing functionality) to improve the isolation of external plugins and make it less likely that such bugs are introduced. In addition, it aims to split functionality into different types of classloaders which are currently merged together to make them easier to understand. E.g. the `DiscriminatingMultiParentClassloader` should be split into two classes `FilteringClassloader` and `MultiParentClassloader` which can be composed to achieve the same result.

Outline of the current classloader hierarchy:

![classloaders_old drawio](https://github.com/elastic/apm-agent-java/assets/6066661/755f1d20-224f-4ab3-9957-0c59d4f4f157)

1. Agent CL: Contains the actual agent code (e.g. agent-core) and dependencies (e.g. bytebuddy). Also currently loads the `plugin-sdk` used by external plugins.
2. `IndyPluginCL` (A) and (b): Contains the classes from the instrumentations that need to be linked to both the classes being instrumented and the agent api's (e.g. Advice classes).
3. `DiscriminatingMultiParentCL (A)` and `(B)`: Controls the source of classes for the `IndPluginCL`:
- First scans the `Agent CL` for internal plugins or the `ExternalPluginCl`for external plugins when searching for a class. Hereby, a filtering is applied to hide certain classes (e.g. the Otel-API embedded in the agent or slf4j / log4j).
- If the class is not found in the first parent (because it does not exist there or is filtered out) an attempt is made to lookup the class from the target application classloader (`App A CL` and `App B CL`).
4. `ExternalPluginCL` can load all classes from the external plugin's JAR file
5. `DiscriminatingMultiParentCL (C)`: is misused (with only a single parent) to hide certain classes (only opentelemetry atm) from external plugins. This isolation is leaky: E.g. using `logback` in a `@GlobalState` class would bind to the agent's logback, which is supposed to be an implementation detail.

I would propose to implement the following classloader structure instead:

![classloaders_new drawio](https://github.com/elastic/apm-agent-java/assets/6066661/4582f755-20fc-46ba-9b5a-62887d73d348)

* The Agent Classloader is split into two classloaders: `Agent CL` and `Plugin SDK CL`. The `Plugin SDK CL` contains our plugin SDK plus dependencies (e.g. bytebuddy). This removes the need for the misused `DiscriminatingMultiParentCl (C)` and ensures proper isolation. If new dependencies are added in the future, it ensures that we don't forget to update the `DiscriminatingMultiParentCl (C)`, because it is not required anymore
* The `DiscriminatingMultiParentCL`s has been split into two simpler CLs : `MultiParent CL` and `Filtering CL`. Because filtering is only required for internal plugins due to the proper isolation of external plugins stated above, this ensures that bugs such as #3145 do not occur anymore.

Feedback on this suggestion is more than welcome!

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.