OSGi classloading workaround + refactor servlet contants by SylvainJuge · Pull Request #2418 · elastic/apm-agent-java

SylvainJuge · 2022-01-26T12:47:31Z

~~Add debug logging to help understanding what could cause #2104~~

UPDATE: re-purposing this PR

avoid loading our Servlet instrumentation with OSGi bundles that only import parts of the Servlet API.
refactor servlet instrumentation constants.
avoid SecurityException thrown by Apache Sling

Fixes #2104

ghost · 2022-01-26T12:58:46Z

💚 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-17T08:58:58.586+0000
Duration: 52 min 0 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2745
Skipped	20
Total	2765

💚 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!)

…sgi-classloading

apm-agent-core/src/test/java/co/elastic/apm/agent/logging/LoggingConfigurationTest.java

…ingConfigurationTest.java Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

…sgi-classloading

SylvainJuge · 2022-02-24T14:30:25Z

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/Constants.java

+                // OSGi bundles might only include 'jakarta.servlet' and not 'jakarta.servlet.http' (same for `java.servlet`), hence
+                // this allows to disable instrumentation as it is required for our instrumentation code.
+                .and(CustomElementMatchers.classLoaderCanLoadClass(osgiClassloaderFilterClass));


[for reviewer] this is what provides the work-around for OSGi by requiring ability to load a class in javax.servlet.http.* or jakarta.servlet.http.* packages.

This workaround assumes that at least one class on the execution stack is loaded by a class loader that knows both javax.servlet and javax.servlet.http. So far it seems to be a valid concept, but let's keep that in mind as a possible source for future reports on missing endpoints.

SylvainJuge · 2022-03-16T14:34:50Z

I have also added a simple work-around for Apache Sling throwing SecurityException, see #2104 (comment) for context.

SylvainJuge · 2022-03-16T14:37:32Z

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

-        ClassLoader servletContextClassLoader = adapter.getClassLoader(servletContext);
+        ClassLoader servletContextClassLoader = null;
+
+        if (!servletContext.getClass().getName().startsWith("org.apache.sling")) {


[for reviewer] we explicitly exclude Apache sling implementation here (and not in the adapter) as it allows to also cover the case where it's migrated to jakarta.servlet, The downside of this is that the service name detection might not work as expected, but with Sling it creates 7+ distinct services for a simple demo app, thus we can expect users to set their service_name explicitly.

…sgi-classloading

github-actions · 2022-03-16T15:56:27Z

/test

eyalkoren

I really like the refactoring! 👏
A negative added-removed balance in PR lines is a great indication for something good 😄

The main comment is about losing class loader references within class loaders. I prefer we only do that on special cases. Class loaders refs within class loaders should represent the delegation hierarchy as much as possible in my opinion.

eyalkoren · 2022-03-17T06:14:43Z

...rc/main/java/co/elastic/apm/agent/bci/classloading/DiscriminatingMultiParentClassLoader.java

+    @Override
+    public String toString() {
+        return "DiscriminatingMultiParentClassLoader{" +
+            "agentClassLoader = " + parents.get(0) + " discriminator = "+ discriminators.get(0) +
+            ", targetClassLoader =" + parents.get(1) + " discriminator = " + discriminators.get(1) +
+            '}';
+    }


...gent-core/src/main/java/co/elastic/apm/agent/bci/classloading/ExternalPluginClassLoader.java

eyalkoren · 2022-03-17T06:17:09Z

...gent-core/src/main/java/co/elastic/apm/agent/bci/classloading/ExternalPluginClassLoader.java

+    public String toString() {
+        return "ExternalPluginClassLoader{" +
+            "pluginJar=" + pluginJar +
+            ", agentClassLoader=" + agentClassLoader +


Could that be something other than the single agent CL?

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

eyalkoren · 2022-03-17T06:36:54Z

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/Constants.java

+                // OSGi bundles might only include 'jakarta.servlet' and not 'jakarta.servlet.http' (same for `java.servlet`), hence
+                // this allows to disable instrumentation as it is required for our instrumentation code.
+                .and(CustomElementMatchers.classLoaderCanLoadClass(osgiClassloaderFilterClass));


This workaround assumes that at least one class on the execution stack is loaded by a class loader that knows both javax.servlet and javax.servlet.http. So far it seems to be a valid concept, but let's keep that in mind as a possible source for future reports on missing endpoints.

apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/Constants.java

eyalkoren · 2022-03-17T06:39:24Z

...m-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/JakartaAsyncInstrumentation.java

-        String servletResponseClassName() {
-            return "jakarta.servlet.ServletResponse";
+        public Constants.ServletImpl getImplConstants() {
+            return Constants.ServletImpl.JAKARTA;


That's nice!

draft printf debugging

69e2ada

github-actions bot added the agent-java label Jan 26, 2022

SylvainJuge added 3 commits January 26, 2022 14:06

use proper logging instead

2e907db

Merge branch 'main' of github.com:elastic/apm-agent-java into debug-o…

d4c01a2

…sgi-classloading

fix log4j classloader name lookup

fa3a2cb

eyalkoren suggested changes Jan 27, 2022

View reviewed changes

apm-agent-core/src/test/java/co/elastic/apm/agent/logging/LoggingConfigurationTest.java Outdated Show resolved Hide resolved

Update apm-agent-core/src/test/java/co/elastic/apm/agent/logging/Logg…

ad21199

…ingConfigurationTest.java Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>

SylvainJuge added the blocked label Feb 2, 2022

SylvainJuge added 2 commits February 24, 2022 13:37

Merge branch 'main' of github.com:elastic/apm-agent-java into debug-o…

a155cf0

…sgi-classloading

refactor servlet constants + add osgi work-around

a847ea4

SylvainJuge added the ci:agent-integration Enables agent integration tests in build pipeline label Feb 24, 2022

SylvainJuge changed the title ~~[debug] osgi classloading debug~~ Feb 24, 2022

SylvainJuge commented Feb 24, 2022

View reviewed changes

SylvainJuge mentioned this pull request Feb 24, 2022

AEM application - traces not showing/capturing outgoing HTTP requests #2104

Closed

prevent SecurityException with Apache sling

5aa9bb5

SylvainJuge commented Mar 16, 2022

View reviewed changes

SylvainJuge removed the blocked label Mar 16, 2022

SylvainJuge added 2 commits March 16, 2022 16:28

Merge branch 'main' of github.com:elastic/apm-agent-java into debug-o…

0aba478

…sgi-classloading

update changelog

bd1362c

SylvainJuge marked this pull request as ready for review March 16, 2022 15:31

SylvainJuge requested a review from eyalkoren March 16, 2022 15:33

eyalkoren reviewed Mar 17, 2022

View reviewed changes

eyalkoren approved these changes Mar 17, 2022

View reviewed changes

SylvainJuge added 3 commits March 17, 2022 09:49

remove useless ref to parent CL

4dc3cad

add *Matcher suffix for extra clarity

3ddf170

simplify + add CL toString()

6e860ff

SylvainJuge enabled auto-merge (squash) March 17, 2022 09:01

SylvainJuge merged commit cb870b0 into elastic:main Mar 17, 2022

eyalkoren mentioned this pull request Mar 27, 2022

Fixing JakartaInitServiceNameInstrumentation #2546

Merged

SylvainJuge deleted the debug-osgi-classloading branch March 28, 2022 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSGi classloading workaround + refactor servlet contants#2418

OSGi classloading workaround + refactor servlet contants#2418
SylvainJuge merged 13 commits intoelastic:mainfrom
SylvainJuge:debug-osgi-classloading

SylvainJuge commented Jan 26, 2022 •

edited by zube bot

Loading

ghost commented Jan 26, 2022 •

edited by ghost

Loading

Build stats

Test stats 🧪

Uh oh!

SylvainJuge Feb 24, 2022

eyalkoren Mar 17, 2022

SylvainJuge commented Mar 16, 2022

SylvainJuge Mar 16, 2022

github-actions bot commented Mar 16, 2022

eyalkoren left a comment

eyalkoren Mar 17, 2022

Uh oh!

eyalkoren Mar 17, 2022

Uh oh!

eyalkoren Mar 17, 2022

Uh oh!

eyalkoren Mar 17, 2022

Labels

2 participants

Conversation

SylvainJuge commented Jan 26, 2022 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ghost commented Jan 26, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

SylvainJuge Feb 24, 2022

Choose a reason for hiding this comment

eyalkoren Mar 17, 2022

Choose a reason for hiding this comment

SylvainJuge commented Mar 16, 2022

SylvainJuge Mar 16, 2022

Choose a reason for hiding this comment

github-actions bot commented Mar 16, 2022

eyalkoren left a comment

Choose a reason for hiding this comment

eyalkoren Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

eyalkoren Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

eyalkoren Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

eyalkoren Mar 17, 2022

Choose a reason for hiding this comment

Labels

2 participants

SylvainJuge commented Jan 26, 2022 •

edited by zube bot

Loading

ghost commented Jan 26, 2022 •

edited by ghost

Loading