First POC for external extension loading #2881

iNikem · 2021-04-28T13:21:04Z

Alternative approach to #2774

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

laurit · 2021-04-29T12:12:29Z

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

+  private static byte[] remapClassBytes(InputStream in) throws IOException {
+    ClassWriter cw = new ClassWriter(0);
+    ClassReader cr = new ClassReader(in);
+    cr.accept(new ClassRemapper(cw, remapper), ClassReader.EXPAND_FRAMES);


out of curiosity is EXPAND_FRAMES really needed for remapping?

No idea, this was copy-paste from ExporterClassLoader

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

iNikem · 2021-05-06T12:56:56Z

I think this PR is now ready for review and then merge. It is still an open question, should we go with URLHandler or move everything into classloader itself. But in anyway I think we should have one way to do that, both in this new classloader and in AgentClassloder. Thus discussion in #2912 is very relevant here as well (/cc @laurit )

laurit · 2021-05-07T08:25:19Z

javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java

-    Constructor constructor =
-        loaderClass.getDeclaredConstructor(URL.class, String.class, ClassLoader.class);
-    return (ClassLoader) constructor.newInstance(bootstrapUrl, innerJarFilename, agentParent);
+    Constructor<ClassLoader> constructor =


is suspect that reflection isn't really necessary here, could probably just use new AgentClassLoader

laurit · 2021-05-07T08:29:32Z

javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/AgentInitializer.java

+    ClassLoader agentClassLoader =
+        constructor.newInstance(bootstrapUrl, innerJarFilename, agentParent);
+
+    Class<?> extensionClassLoaderClass =


reflection here could be avoided by either doing this inside AgentInstaller or moving ExtensionClassLoader out of inst/

I will touch this code again when doing support for multiple extension jars.

laurit · 2021-05-07T10:32:25Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

+    URL extension =
+        parseLocation(
+            System.getProperty(
+                "otel.javaagent.experimental.extensions",


System property is named extensions but seems like it only accepts 1 file. Should it be possible to only add 1 extension or multiple?
If multiple do they all live in the same class loader or would each get its own?

As class comment says, we want to support scanning the folder for several jars. In that case every jar should get a separate classloader, I think. This is a goal for a subsequent PR.

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java

anuraaga · 2021-05-13T06:55:50Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

+  }
+
+  public static ClassLoader getInstance(ClassLoader parent) {
+    // TODO add support for old deprecated property otel.javaagent.experimental.exporter.jar


Dunno if it's worth it vs just keeping ExporterClassLoader for a little bit

anuraaga · 2021-05-13T06:57:12Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

+      extension =
+          parseLocation(
+              System.getProperty(
+                  "otel.javaagent.experimental.initializer.jar",


Similarly, I'd probably just leave our current code until we get rid of the property instead of complicating this class with backwards compatibility.

When I add support for several jars, this will not be a big complication...

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

anuraaga · 2021-05-13T07:01:47Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlConnection.java

+  private final JarFile delegateJarFile;
+  private final JarEntry entry;
+
+  private InputStream cachedResult;


Is only one URLConnection ever instantiated? Not sure when openConnection is called during classload

Certainly not one, why do you ask?

If not once, I guess this cache is never used? Should it be a static cache?

A, that's the catch :) it IS read/called at least 2 times by URLClassoader. Old AgentClassLoader also had an optimisation for this double reading.

Caching Inputstream this way is weird. As streams can be read only once without using mark/reset then this caching would make sense only when someone opens the stream and doesn't use it at all and then reopens the stream and actually reads it. Perhaps this isn't really needed any more? If it is needed then caching byte array and returning a new ByteArrayInputStream from getInputStream would be nicer.

handler.getInputStream() is called twice:

In jdk.internal.loader.URLClassPath.Loader#getResource(java.lang.String, boolean), where exactly "someone opens the stream and doesn't use it at all" happens

in jdk.internal.loader.Resource#cachedInputStream, when stream is actually used afterwards.

But I agree with you that caching byte arrays makes more sense, will do it.

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

examples/extension/smoke-tests/src/test/java/com/example/javaagent/smoketest/SmokeTest.java

examples/extension/custom/build.gradle

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlConnection.java

examples/extension/custom/build.gradle

anuraaga

A bit uncertain of the caching but basically looks good

trask

ExtensionClassLoader's URLStreamHandler/URLConnection approach has diverged from the new AgentClassLoader behavior implemented by @laurit in #2912, it's not clear if these differences are needed, or it's just drift since you started this work prior to his changes?

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java

examples/extension/build.gradle

examples/extension/custom/src/main/java/com/example/javaagent/DemoIdGenerator.java

examples/extension/settings.gradle

trask · 2021-05-16T02:08:02Z

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

+        return null;
+      }
+
+      // That will NOT remap the content of files under META-INF/services


is this a problem?

Not for now

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java

iNikem · 2021-05-17T06:52:20Z

ExtensionClassLoader's URLStreamHandler/URLConnection approach has diverged from the new AgentClassLoader behavior implemented by @laurit in #2912, it's not clear if these differences are needed, or it's just drift since you started this work prior to his changes?

We discussed this with Lauri and agreed that it is Ok for now to have different ways to do things. I will review it later.

trask · 2021-05-17T07:04:19Z

I will review it later.

👍

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlConnection.java

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java

laurit · 2021-05-18T08:23:35Z

examples/extension/smoke-tests/src/test/java/com/example/javaagent/smoketest/SmokeTest.java

+  static void setupSpec() {
+    backend =
+        new GenericContainer<>(
+            "open-telemetry-docker-dev.bintray.io/java/smoke-fake-backend:latest")


Smoketests use ghcr.io/open-telemetry/java-test-containers:smoke-fake-backend-20210324.684269693

trask · 2021-05-19T01:35:33Z

🥳

iNikem added 10 commits April 28, 2021 11:08

First POC for external extension loading

dd7eb31

Fix NPE

107564c

Urlhandler

f1c491e

Fix

6bbedd7

Polish

0d0c679

Polish

dc4c9b3

Urlhandler

ee098d3

Trying Urlhandler

2b3a297

Alternative way to load extensions

78620ca

Polish

428e932

iNikem requested review from anuraaga, jkwatson, laurit, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners April 28, 2021 13:21

laurit reviewed Apr 29, 2021

View reviewed changes

iNikem added 2 commits May 5, 2021 16:07

Add example extensions and smoke test for their usage

4171453

Support for system property

1c6d0e0

iNikem marked this pull request as draft May 6, 2021 11:04

Polish

42321b1

iNikem marked this pull request as ready for review May 6, 2021 12:55

iNikem requested a review from laurit May 6, 2021 12:57

laurit reviewed May 7, 2021

View reviewed changes

iNikem added 3 commits May 10, 2021 14:58

Avoid doing remapping twice

fa06f43

Merge remote-tracking branch 'upstream/main' into urlhandler

d148eb1

Polish after merge

9910497

anuraaga reviewed May 13, 2021

View reviewed changes

mateuszrzeszutek approved these changes May 13, 2021

View reviewed changes

iNikem added 3 commits May 14, 2021 12:55

Merge remote-tracking branch 'upstream/main' into urlhandler

f768e8a

Update to latest changes in extentions api

bab28b4

Polish

54497ae

mateuszrzeszutek approved these changes May 14, 2021

View reviewed changes

examples/extension/custom/build.gradle Outdated Show resolved Hide resolved

No need for tooling

17a5576

anuraaga approved these changes May 15, 2021

View reviewed changes

trask approved these changes May 16, 2021

View reviewed changes

Simplify

f7e8785

Cleanup

424d0b3

laurit reviewed May 17, 2021

View reviewed changes

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlConnection.java Outdated Show resolved Hide resolved

laurit reviewed May 17, 2021

View reviewed changes

...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/RemappingUrlStreamHandler.java Outdated Show resolved Hide resolved

laurit reviewed May 18, 2021

View reviewed changes

laurit approved these changes May 18, 2021

View reviewed changes

iNikem added 3 commits May 18, 2021 11:36

Better error handling

7a27cc3

Merge remote-tracking branch 'upstream/main' into urlhandler

9c8ce19

Update to the latest API

d5844a4

iNikem merged commit e3cf8ec into open-telemetry:main May 18, 2021

iNikem deleted the urlhandler branch May 18, 2021 19:59

jsuereth mentioned this pull request May 27, 2021

sample launch command in documentation is no longer correct for v1.x of opentelemetry-javaagent.jar GoogleCloudPlatform/opentelemetry-operations-java#106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First POC for external extension loading #2881

First POC for external extension loading #2881

iNikem commented Apr 28, 2021

laurit Apr 29, 2021

iNikem May 5, 2021

iNikem commented May 6, 2021

laurit May 7, 2021

laurit May 7, 2021

iNikem May 13, 2021

laurit May 7, 2021

iNikem May 13, 2021

anuraaga May 13, 2021

anuraaga May 13, 2021

iNikem May 14, 2021

anuraaga May 13, 2021

iNikem May 14, 2021

anuraaga May 15, 2021

iNikem May 15, 2021

laurit May 17, 2021

iNikem May 18, 2021

anuraaga left a comment

trask left a comment

trask May 16, 2021

iNikem May 17, 2021

iNikem commented May 17, 2021

trask commented May 17, 2021

laurit May 18, 2021

trask commented May 19, 2021

First POC for external extension loading #2881

First POC for external extension loading #2881

Conversation

iNikem commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented May 17, 2021

trask commented May 17, 2021

Choose a reason for hiding this comment

trask commented May 19, 2021