First POC for external extension loading #2774

iNikem · 2021-04-12T07:35:28Z

Please start reading from ExtensionClassLoader.

This is the first attempt to address #2442. Manual testing confirmed that this works.

trask

any thoughts on class loader isolation, so extensions can bring their own version of grpc/netty and not cause conflicts? or should we table that aspect for now

trask · 2021-04-13T02:45:20Z

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

+ * This classloader is used to load arbitrary extensions for Otel Java instrumentation agent. Such
+ * extensions may include SDK components (exporters or propagators) and additional instrumentations.
+ * They have to be isolated and shaded to reduce interference with the user application and to make
+ * it compatible with shaded SDK used by the agent.


Suggested change

* it compatible with shaded SDK used by the agent.

* it compatible with shaded API used by the agent.

trask · 2021-04-13T02:47:09Z

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

+          rule(
+              "#io.opentelemetry.extension.aws",
+              "#io.opentelemetry.javaagent.shaded.io.opentelemetry.extension.aws"),


I forget why this needs to be shaded..

This is the x-ray propagator - I think it's to be able to inject into user classloader for use from instrumentation, even if they have their own (potentially incompatible) one. Since extension instrumentation may use the propagator it seems appropriate to shade here too.

trask · 2021-04-13T02:55:27Z

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

+    // TODO add support for system properties
+    URL extension = parseLocation(System.getenv("OTEL_JAVAAGENT_EXTENSIONS"));
+    if (extension != null) {
+      return new ExtensionClassLoader(extension, parent);


should we only handle OTEL_JAVAAGENT_EXTENSIONS pointing to a directory, and use all *.jar in that directory? we will want that option I think for easy dropping in of multiple extensions, and may be reasonable to only support that option (simpler doc story not to have multiple meanings of OTEL_JAVAAGENT_EXTENSIONS)

I would support both single file and a directory.

trask · 2021-04-13T03:16:09Z

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

+
+  public ExtensionClassLoader(URL url, ClassLoader parent) {
+    super(new URL[] {url}, parent);
+    this.manifest = getManifest(url);


oh, I remember this, I'm not sure how to generalize this to more than one jar file under the constraint of using URLClassLoader

maybe we can pass URLs to URLClassLoader that have custom InputStreams that do the shading? If that can work somehow (big if) then URLClassLoader will take care of the class/package definitions for us and we don't need the manifest

Hm... in fact we can do this, I think... I will give it a try.

iNikem · 2021-04-13T06:14:35Z

What isolation of what from what are you thinking about? :) One extension jar from another? Extensions from AgentClassLoader?

trask · 2021-04-13T07:17:16Z

What isolation of what from what are you thinking about? :) One extension jar from another? Extensions from AgentClassLoader?

these are good questions 😁, ideally all of the above(?), but don't want to over-engineer it either

iNikem · 2021-04-13T09:49:48Z

Extensions are already loaded from a separate classloader, which is a child of AgentClassLoader. Meaning nobody should see their classes. Extensions themselves should have access to agent classes, thus AgentClassLoader as a parent. This current state + possible separating extensions from each other - is it Ok or do we want more?

mateuszrzeszutek · 2021-04-29T14:02:32Z

examples/extension/smoke-tests/build.gradle

+  inputs.files(layout.files(shadowTask))
+
+  doFirst {
+    jvmArgs("-Dio.opentelemetry.smoketest.agent.shadowJar.path=/Users/nsalnikovtarnovski/Documents/workspace/opentelemetry-java-instrumentation/javaagent/build/libs/opentelemetry-javaagent-1.2.0-SNAPSHOT-all.jar")


You probably need to pass some other javaagent path here 😄

mateuszrzeszutek · 2021-04-29T14:06:26Z

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

@@ -34,21 +34,27 @@ private static synchronized void startAgent(Instrumentation inst, URL bootstrapU
      throws Exception {
    if (AGENT_CLASSLOADER == null) {
      ClassLoader agentClassLoader = createAgentClassLoader("inst", bootstrapUrl);
+      AGENT_CLASSLOADER = agentClassLoader;
+
      Class<?> agentInstallerClass =
          agentClassLoader.loadClass("io.opentelemetry.javaagent.tooling.AgentInstaller");


If agentClassLoader is an ExtensionClassLoader then won't we put AgentInstaller in the same CL as extensions? And won't it apply shading transformations on all agent classes?

No, because ExtensionClassLoader first delegates to parent, as usual. And its parent is the old AgentClassLoader

mateuszrzeszutek · 2021-04-29T14:12:19Z

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

+
+  @Override
+  public InputStream getResourceAsStream(String name) {
+    InputStream originalStream = super.getResourceAsStream(name);


Missing if (originalStream == null)

iNikem requested review from anuraaga, jkwatson, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners April 12, 2021 07:35

trask reviewed Apr 13, 2021

View reviewed changes

iNikem force-pushed the extensions branch 2 times, most recently from 9104930 to 27f17cd Compare April 13, 2021 17:44

iNikem requested a review from laurit as a code owner April 23, 2021 13:36

iNikem added 6 commits April 28, 2021 11:08

First POC for external extension loading

dd7eb31

Fix NPE

107564c

Urlhandler

f1c491e

Fix

6bbedd7

Polish

0d0c679

Polish

dc4c9b3

iNikem force-pushed the extensions branch from 1c5e702 to dc4c9b3 Compare April 28, 2021 12:16

iNikem mentioned this pull request Apr 28, 2021

First POC for external extension loading #2881

Merged

Add example of building and using extensions

2a8d75d

mateuszrzeszutek reviewed Apr 29, 2021

View reviewed changes

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

iNikem closed this May 18, 2021

iNikem deleted the extensions branch July 18, 2021 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First POC for external extension loading #2774

First POC for external extension loading #2774

iNikem commented Apr 12, 2021

trask left a comment

trask Apr 13, 2021

trask Apr 13, 2021

anuraaga Apr 13, 2021

trask Apr 13, 2021

iNikem Apr 13, 2021

trask Apr 13, 2021

iNikem Apr 13, 2021

iNikem commented Apr 13, 2021

trask commented Apr 13, 2021

iNikem commented Apr 13, 2021

mateuszrzeszutek Apr 29, 2021

mateuszrzeszutek Apr 29, 2021

iNikem Apr 29, 2021

mateuszrzeszutek Apr 29, 2021

	* it compatible with shaded SDK used by the agent.
	* it compatible with shaded API used by the agent.

First POC for external extension loading #2774

First POC for external extension loading #2774

Conversation

iNikem commented Apr 12, 2021

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

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

iNikem commented Apr 13, 2021

trask commented Apr 13, 2021

iNikem commented Apr 13, 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