Refactor Servlet API and JDBC plugins to use a MethodHandles-based advice dispatching approach by felixbarny · Pull Request #1090 · elastic/apm-agent-java

felixbarny · 2020-03-20T15:55:11Z

First step of #1085

First step of elastic#1085

…ce-via-method-handle

…her class loader Remove unnecessary helpers from servlet instrumentation

Removed @VisibleForAdvice

- Helper CL unloading (GC) test - polishing

felixbarny · 2020-03-25T11:51:05Z

...pm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/FilterChainInstrumentation.java

+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    private static Transaction onEnterServletService(@Advice.Origin Class<?> clazz,
+                                                     @Advice.Argument(0) ServletRequest servletRequest) throws Throwable {
+        return (Transaction) MethodHandleDispatcher


Referencing agent types in advice methods might not be possible anymore once we load the agent from an isolated class loader.

…ce-via-method-handle

codecov-io · 2020-03-25T16:39:19Z

Codecov Report

Merging #1090 into master will not change coverage by %.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1090   +/-   ##
=========================================
  Coverage     58.52%   58.52%           
  Complexity       87       87           
=========================================
  Files           334      334           
  Lines         15753    15753           
  Branches       2253     2253           
=========================================
  Hits           9220     9220           
  Misses         5912     5912           
  Partials        621      621

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0e0abd...7810901. Read the comment docs.

…ce-via-method-handle

eyalkoren

Looks really good! 👏
One suggestion of making it safer with postponing helper class resolution and a request for extending the class-leak tests.
Other than that - cosmetic stuff, mostly trying to make it easier to use right and understand.
Well done!

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/HelperClassManager.java

eyalkoren · 2020-04-06T05:26:57Z

apm-agent-core/src/main/java/co/elastic/apm/agent/bootstrap/MethodHandleDispatcher.java

+    }
+
+    @Nullable
+    public static ConcurrentMap<String, MethodHandle> getDispatcherForClassLoader(@Nullable ClassLoader classLoader) {


Terminology- in general, there are lots of mentions to "dispatcher", but there are no such entities. I suggest you either add such (probably instances of this class) that hold the method handle registries and these instances will be held by the MethodHandleDispatcherHolder (as its name suggests), or you reference them here as registries (rather than "dispatcher") eg setMethodHandleRegistryForClassLoader.

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/HelperClassManager.java

eyalkoren · 2020-04-06T05:58:50Z

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/HelperClassManager.java

+                    Class<?> clazz = Class.forName(name, true, helperCL);
+                    if (clazz.getClassLoader() != helperCL) {
+                        throw new IllegalStateException("Helper classes not loaded from helper class loader, instead loaded from " + clazz.getClassLoader());
+                    }
+                    try {
+                        // call in from the context of the created helper class loader so that helperClass.getMethods() doesn't throw NoClassDefFoundError
+                        Method registerStaticMethodHandles = methodHandleRegistererClass.getMethod("registerStaticMethodHandles", ConcurrentMap.class, ConcurrentMap.class, Class.class);
+                        registerStaticMethodHandles.invoke(null, dispatcherForClassLoader, reflectionDispatcherForClassLoader, clazz);
+                    } catch (Exception e) {
+                        logger.error("Exception while trying to register method handles for {}", name);
+                        if (e instanceof InvocationTargetException) {
+                            Throwable targetException = ((InvocationTargetException) e).getTargetException();
+                            if (targetException instanceof Exception) {
+                                throw (Exception) targetException;
+                            }
+                        }
+                        throw e;
+                    }


This part may be done prematurely - it will be invoked when the bytecode-injection candidate class is found, however the helper may depend on other classes that are not yet loaded and loading them here is a side effect. The use of reflection means completion of the helper classes linkage, which kind of guarantees that. Related errors will be very hard to detect.
I think that this part should be done lazily, meaning - keep a mapping between the helper class name and the helperCL and do the actual registration only upon first invocation of the injected method. Should be a lot safer.

Lazily injecting the helper classes has the downside that we wouldn't be able to leverage an upcoming feature in Byte Buddy. This feature would inject an INVOKEDYNAMIC instruction to call the helper method: raphw/byte-buddy#830 (comment)
This avoids the overhead of having to look up the method handle from a map.

However, there are a few things we can do to make this safer:

We already only load classes where we checked with Byte Buddy matchers that they contain @RegisterMethodHandle-annotated methods.

Call getMethods instead of getDeclaredMethods: avoids resolving types of all parameters of non-public methods. So we could make methods with problematic signatures private or package-private.

Using the upcoming bootstrap method, I think we'd be able to get the method handles without having to use reflection. We'd need to do some testing but I suspect we'll need a fallback for Java 7 that does not rely on INVOKEDYNAMIC.

Lazily injecting the helper classes has the downside that we wouldn't be able to leverage an upcoming feature in Byte Buddy

Is it something we should rely on? And won't we have to do some adjustments anyhow?

Call getMethods instead of getDeclaredMethods: avoids resolving types of all parameters of non-public methods. So we could make methods with problematic signatures private or package-private.

The nature of these problems is that they are very setup-dependant, so there are not "problematic signatures" per se. In most cases it will work, but somewhere (eg some module-dependency systems) these side effects may cause a lookup of a type that is not yet available.

Bottom line- it is safer and if the effort is not big, I think it worth it (as the effort to debug even a single related problem could be bigger). However, if the effort is substantial, or if you think it will require a rework effort in the foreseeable future - let's merge as is and rethink later if needed. It's really your call, either way is fine for me.

eyalkoren · 2020-04-06T06:17:23Z

apm-agent-core/src/main/java/co/elastic/apm/agent/util/CallDepth.java

+import java.util.WeakHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+
+// TODO docs


apm-agent-core/src/test/java/co/elastic/apm/agent/util/PackageScannerTest.java

eyalkoren · 2020-04-06T06:55:18Z

...ent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/JdbcInstrumentation.java

-        }
+    @Override
+    public List<String> helpers() throws Exception {
+        return PackageScanner.getClassNames(getClass().getPackage().getName());


Why not be more specific (only real helper classes)?

My vision for an ideal situation (which requires some changes in Byte Buddy) is that advices can be written without having to manually get a MethodHandle. When we load all methods from the helper class loader, you can even implement an instrumentation with just one file, without having to worry about which classes are able to access application classes.
After all plugins have been refactored, we can pull this method up, making it the default behavior to include all classes.

you can even implement an instrumentation with just one file

In general, a separation will reduce the chances for errors. It's always better if you have a class exactly once in your classpath. Consider such an instrumentation class with a static field, or some initialization code run in the instrumentation constructor. What can actually be seen in runtime from these annotated method is very difficult to understand when just looking at such file.

Again- not a blocker.

apm-agent-core/src/main/java/co/elastic/apm/agent/util/PackageScanner.java

eyalkoren · 2020-04-06T07:02:15Z

...ervlet-plugin/src/main/java/co/elastic/apm/agent/servlet/AbstractServletInstrumentation.java


+    @Override
+    public List<String> helpers() throws Exception {
+        return PackageScanner.getClassNames(getClass().getPackage().getName());


Same- since this would be a reference for future usages, I think it's better to extract the @RegisterMethodHandle methods to dedicated classes in dedicated packages. It's easier to make mistakes if we mix instrumentations and advices with those. It also makes sense that packages reflect the complicated class loading contexts.

…ce-via-method-handle

eyalkoren

Can be merged as is if all tests work. I would run some manual tests in addition in specific scenarios, eg using the attach methods as well, or JDBC on real servlet container.

felixbarny · 2020-06-15T07:39:49Z

superseded by #1230

felixbarny added 7 commits March 20, 2020 16:54

Invoke ServletApiAdvice via MethodHandles

285b488

First step of elastic#1085

Merge remote-tracking branch 'origin/master' into invoke-servlet-advi…

10c1c6b

…ce-via-method-handle

Load all classes within a plugin from a HelperClassManager.ForDispatc…

6681d2e

…her class loader Remove unnecessary helpers from servlet instrumentation

Only register @RegisterMethodHandle annotated methods

cb11904

Removed @VisibleForAdvice

Improve diagram

554bf88

Refactor jdbc plugin to use MethodHandleDispatcher

e06680c

Don't create helper CL for plugins by default

06375d8

- Helper CL unloading (GC) test - polishing

felixbarny changed the title ~~Invoke ServletApiAdvice via MethodHandles~~ Refactor Servlet API and JDBC plugins to use a MethodHandles-based advice dispatching approach Mar 25, 2020

Avoid Class.getPackageName() as it's Java 9+

ea9db90

felixbarny commented Mar 25, 2020

View reviewed changes

Fix bugs discovered by integration tests

47a25f4

felixbarny mentioned this pull request Mar 25, 2020

Customizable advice dispatcher raphw/byte-buddy#830

Closed

felixbarny added 2 commits March 25, 2020 16:07

Fix postgres driver related bug

84ba2b6

Merge remote-tracking branch 'origin/master' into invoke-servlet-advi…

a9a4611

…ce-via-method-handle

felixbarny added 2 commits March 26, 2020 11:00

Avoid FileSystemAlreadyExistsException when scanning package of jar

8287bcb

Avoid NoClassDefFoundErrors

daa8e29

SylvainJuge assigned felixbarny Mar 30, 2020

Reflection-based dispatching for Java 7

edf8ffe

felixbarny marked this pull request as ready for review March 31, 2020 06:46

Merge remote-tracking branch 'origin/master' into invoke-servlet-advi…

a8a08e9

…ce-via-method-handle

felixbarny added the [zube]: In Review label Apr 1, 2020

eyalkoren reviewed Apr 6, 2020

View reviewed changes

felixbarny added 2 commits April 6, 2020 18:04

Apply suggestions from code review

c0f3750

Merge remote-tracking branch 'origin/master' into invoke-servlet-advi…

7810901

…ce-via-method-handle

eyalkoren approved these changes Apr 7, 2020

View reviewed changes

felixbarny mentioned this pull request Jun 15, 2020

Indy plugins #1230

Merged

felixbarny closed this Jun 15, 2020

zube bot removed the [zube]: In Review label Jun 15, 2020

zube bot added the [zube]: Done label Jun 15, 2020

zube bot removed the [zube]: Done label Oct 13, 2020

Conversation

felixbarny commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

felixbarny commented Jun 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

felixbarny commented Mar 20, 2020 •

edited

Loading

codecov-io commented Mar 25, 2020 •

edited

Loading