[Rejected] "java -ext.dirs" is not supported

jackiepon31 commented 5 years ago

Please answer these questions before submitting your issue.

Why do you submit this issue?
[ ] Question or discussion
[ ] Bug
[ 1] Requirement
[ ] Feature or performance improvement

Question

What do you want to know? I just want to fix something to the AgentClassLoader

Bug

Which version of SkyWalking, OS and JRE?
Which company or project?
What happen? In some cases, especially when the target java app is packaged in an old way, i mean the depency libs directory is out of the target application jar. We need to add some VM options to make sure the java applications starts well, like "java -cp /lib -ext.dirs /lib" . But when we add these VM options, the target ClassLoaders may not be able to load any classes from skywalking-agent.jar cause it's out of the path i have defined previously. The target app classloader or any of its parent classloader will load the class with failure and push it down to AgentClassLoader to load the class ,and finally ,the not class found exception will stop the application from start.

Requirement or improvement
Please describe about your requirements or improvement suggestions.

I suggest to add SkyWalking-agent.jar or its dirs to classpath to make the load mechanism compatible with old packaing way instead of spring-boot fat jar.

wu-sheng commented 5 years ago

Yes. ext-dir is not supported today. But I think -cp should work. If you want to fix this, welcome. We may not have slot to fix this shortly as low priority, little people use this way to package anymore.

wu-sheng commented 5 years ago

@candyleer If you have the interest, welcome to consider this.

jackiepon31 commented 5 years ago

thanks a lot for your answer, yeah，really ware to see this old style of packing, i'm trying to build an APM system for my company , after 5 years of iteration, the Architecture of backend is really really complex and drive me to crazy ,lol so, I'm right now doing some pre-study work on skywalking and i am gonna create some new pluggings and new storage implementations . it's realling chanllenging !! I'm hoping for your support,maybe in the nearby future.

wu-sheng commented 5 years ago

Plugin development is pretty easy in SkyWalking. We have detailed documents for that, read that, you should be fine. Just in open source, there are dozens, even over 100+ contributors are/were working on SkyWalking plugins, so, don't consider it is complex. SkyWalking's core has taken care of the complex part, it is just a more powerful AOP style core already.

jackiepon31 commented 5 years ago

cool, thanks for your advice , how can i join u guys as a contributors ?

wu-sheng commented 5 years ago

There is no joining process. :) Contributions are open to everyone.

If you mean to be a committer, I think you could read this, https://github.com/apache/skywalking/blob/master/docs/en/guides/asf/committer.md Simple version, committers are the ones who have proved that they are familiar with the project, being qualified to be an evangelist and reviewer, and could be trusted to help in keeping the project healthy and stable.

wu-sheng commented 5 years ago

Official PMC and committer team are here, http://skywalking.apache.org/team/

jackiepon31 commented 5 years ago

Cool, got that , seems like there 's still a long way for me to go there ,lol Thank u wu sheng , i ' ll try my best to contribute more to skywalking cause i really like this project.

wu-sheng commented 5 years ago

Cool, got that , seems like there 's still a long way for me to go there ,lol

I have to say, yes. Most people got the committer with over 10 PRs merged and 3-5 months. Our latest new committer is @dmsolr :) You could see what he did at

jackiepon31 commented 5 years ago

wow, he really does make a lot of commit recently , i 'll refre to that , thank u

kim-up commented 3 years ago

I have a question: Do you have to load the depency libs by "ExtClassLoader"? @jackiepon31

"-Xbootclasspath/a:" can not solve this problem？By the way ,we can use the BootstrapClassLoader to load the them. We can run the oldWay.jar such as:

java -javaagent:/{skywalking-agent-dir}/skywalking-agent-abm.jar
-Xbootclasspath/a:/{your}.jar
-Dskywalking.agent.service_name=serverName
-Dskywalking.collector.backend_service=127.0.0.1:11800

Maybe  multiple  jar  in one directory:

java -javaagent:/{skywalking-agent-dir}/skywalking-agent-abm.jar
-Xbootclasspath/a:/{jar-directory}
-Dskywalking.agent.service_name=serverName
-Dskywalking.collector.backend_service=127.0.0.1:11800

If don't want to do that like the above Just like @jackiepon31 said： “I suggest to add SkyWalking-agent.jar or its dirs to classpath to make the load mechanism compatible with old packaing way instead of spring-boot fat jar.” by using the BootstrapClassLoader to load the them, rather than the ExtClassLoader ,is OK? @wu-sheng .Because ExtClassLoader can't seem to do it。 We can use "instrumentation.appendToBootstrapClassLoaderSearch(jarFile)":

public class SkyWalkingAgent {
        private static ILog LOGGER = LogManager.getLogger(SkyWalkingAgent.class);
        public static void premain(String agentArgs, Instrumentation instrumentation) throws PluginException {
             File basePath = AgentPackagePath.getPath();
             File classPathDictionary = new File(basePath, "classpath");
             if (classPathDictionary.exists() && classPathDictionary.isDirectory()) {
                    String[] jarFileNames = classPathDictionary.list((dir, name) -> name.endsWith(".jar"));
                    for (String fileName : jarFileNames) {
                        JarFile jarFile = new JarFile(classPathDictionary + "/" + fileName);
                        instrumentation.appendToBootstrapClassLoaderSearch(jarFile);
                    }
              }
       //.......
      }

"classpath" is a dictionary that has the jars which need to load to classpath;

skywalking
      -- classpath
           -- 1.jar
           -- 2.jar
      -- skywalking-agent-abm.jar

wu-sheng commented 3 years ago

@HendSame Hi, first of all, thank you for bringing these suggestion to the community. I like the -Xbootclasspath solution as an alternative way. Could you provide a FAQ doc by a pull request to describe this? I think this would be helpful And speaking of instrumentation.appendToBootstrapClassLoaderSearch(jarFile) solution, I haven't tried this way, I am not sure whether this is working? Because the agent core classes are still in the bootstrap classloader. Do you have tested this?

kim-up commented 3 years ago

1、[FAQ doc] #6273 @wu-sheng 2、About the solution of “instrumentation.appendToBootstrapClassLoaderSearch(jarFile)” .Yeah! I had tested this. It was working. Because we have a project which is named "probe-lib" in our company. And the "probe-lib" would Includes many open source probes,such as "skywalking-agent"、"transmittable-thread-local"(Alibaba's TTL agent). But TTL have to load by bootstrap classloader.So "instrumentation.appendToBootstrapClassLoaderSearch(jarFile)" can load the TTL to classpath by "probe-lib",and skywalking-agent also be load by "probe-lib".They are all working normally.

wu-sheng commented 3 years ago

@HendSame Could you make a test app to run in ext-class-loader directly? And share the test codes, and your SkyWalking fork version? I am looking forward to check that.

kim-up commented 3 years ago

@wu-sheng It is running in bootstrap- class-loader rather than the ext-class-loader. I have not found a solution that can be done by ext-class-loader directly.

wu-sheng commented 3 years ago

I see. Make sense.

kim-up commented 3 years ago

All codes as follows:

import java.io.File;
import java.io.IOException;
import java.util.jar.JarFile;
import org.apache.skywalking.apm.agent.core.boot.AgentPackagePath;

public class SkyWalkingAgent {
    private static ILog LOGGER = LogManager.getLogger(SkyWalkingAgent.class);

    /**
     * Main entrance. Use byte-buddy transform to enhance all classes, which define in plugins.
     */
    public static void premain(String agentArgs, Instrumentation instrumentation) throws PluginException {
        File basePath = null;
        try {
            basePath = AgentPackagePath.getPath();
        } catch (AgentPackageNotFoundException ape) {
            LogManager.getLogger(SkyWalkingAgent.class)
                    .error(ape, "SkyWalking agent class resource path not found.");
            return;
        }
        File classPathDictionary = new File(basePath, "classpath");
        if (classPathDictionary.exists() && classPathDictionary.isDirectory()) {
            String[] jarFileNames = classPathDictionary.list((dir, name) -> name.endsWith(".jar"));
            for (String fileName : jarFileNames) {
                JarFile jarFile = null;
                try {
                    jarFile = new JarFile(classPathDictionary + "/" + fileName);
                } catch (IOException ioe) {
                    LogManager.getLogger(SkyWalkingAgent.class)
                            .error(ioe, "Load jar {} to classpath failure.", fileName);
                }
                instrumentation.appendToBootstrapClassLoaderSearch(jarFile);
            }
        }
         // The original code
      }
   // The original code
}

daimingzhi commented 3 years ago

I built a demo for testing this case and found -Xbootclasspath/a is not able to resolve it

you can found the demo in https://github.com/daimingzhi/skywalkingTestDemo.git

I tested two scenarios：

enhance instance（intercept constructor of instance or intercept method of instance）
enhance class

Scenario one

In the first scenario, the application starts normally, but the enhancement did not take effect, and the following log can be found in file skywalking-api.log

ERROR 2021-08-15 14:18:47:133 main SkyWalkingAgent : Enhance class com.easy4coding.jar.Person error. 
org.apache.skywalking.apm.agent.core.plugin.PluginException: Can't create InstanceMethodsAroundInterceptor.
    at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInter.<init>(InstMethodsInter.java:98)
    at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassEnhancePluginDefine.enhanceInstance(ClassEnhancePluginDefine.java:177)
    at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassEnhancePluginDefine.enhance(ClassEnhancePluginDefine.java:74)
    at org.apache.skywalking.apm.agent.core.plugin.AbstractClassEnhancePluginDefine.define(AbstractClassEnhancePluginDefine.java:77)
    at org.apache.skywalking.apm.agent.SkyWalkingAgent$Transformer.transform(SkyWalkingAgent.java:144)
    at org.apache.skywalking.apm.dependencies.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.doTransform(AgentBuilder.java:10325)
    at org.apache.skywalking.apm.dependencies.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.transform(AgentBuilder.java:10263)
    at org.apache.skywalking.apm.dependencies.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.access$1600(AgentBuilder.java:10029)
    at org.apache.skywalking.apm.dependencies.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer$LegacyVmDispatcher.run(AgentBuilder.java:10648)
    at org.apache.skywalking.apm.dependencies.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer$LegacyVmDispatcher.run(AgentBuilder.java:10595)
    at java.security.AccessController.doPrivileged(Native Method)
    at org.apache.skywalking.apm.dependencies.net.bytebuddy.agent.builder.AgentBuilder$Default$ExecutingTransformer.transform(AgentBuilder.java:10186)
    at sun.instrument.TransformerManager.transform(TransformerManager.java:188)
    at sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428)
    at java.lang.ClassLoader.defineClass1(Native Method)
    at java.lang.ClassLoader.defineClass(ClassLoader.java:763)
    at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
    at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
    at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:411)
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
    at java.lang.Class.forName0(Native Method)
    at java.lang.Class.forName(Class.java:264)
    at com.easy4coding.agent.MainClass.main(MainClass.java:13)
Caused by: java.lang.ClassNotFoundException: Can't find org.apache.skywalking.apm.plugin.tomcat78x.ExtNonStaticInterceptor
    at org.apache.skywalking.apm.agent.core.plugin.loader.AgentClassLoader.findClass(AgentClassLoader.java:118)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
    at java.lang.Class.forName0(Native Method)
    at java.lang.Class.forName(Class.java:348)
    at org.apache.skywalking.apm.agent.core.plugin.loader.InterceptorInstanceLoader.load(InterceptorInstanceLoader.java:71)
    at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInter.<init>(InstMethodsInter.java:96)
    ... 29 more

I debugged the code to find more detailed messages.

We used AgentClassLoader which parent classLoader is ExtClassLoader（because the class we wanted to enhance is loaded by ExtClassLoader）, to load a Interceptor defined in skywalking.jar，but skywalking.jar is loader by AppClassLoader。Therefore,the load will fail with an classNotFoundException thrown

Scenario two

In the second scenario, the application starts failed.

This is because, when a static method is enhanced, ByteBuddy generates a static field for the class. The field is of type StaticMethodsInter, which will be set approach by reflection and this behavior will trigger loading of StaticMethodsInter

however,the enhanced class is loaded by ExtClassLoader，and it can not load StaticMethodsInter from skywalking.jar .

It can be seen from the above，use BootstrapClassLoader to load the class need to enhanced may not take effect。because, the same as ExtClassLoader,BootstrapClassLoader can not load any class from skywalking.jar .

In my demo,I did like this to resolve this problem.

@Override
public boolean isBootstrapInstrumentation() {
   return true;
}

Maybe skywalking can support some configuration to config default return value of this method instead of using "false". For example, We can use Plugin.BootstrapInstrumentation.dirs=apm-sniffer\apm-sdk-plugin\httpClient-4.x-plugin to make all methods named isBootstrapInstrumentation in apm-sniffer\apm-sdk-plugin\httpClient-4.x-plugin directory return true。

wu-sheng commented 3 years ago

Maybe skywalking can support some configuration to config default return value of this method instead of using "false". For example, We can use Plugin.BootstrapInstrumentation.dirs=apm-sniffer\apm-sdk-plugin\httpClient-4.x-plugin to make all methods named isBootstrapInstrumentation in apm-sniffer\apm-sdk-plugin\httpClient-4.x-plugin directory return true。

@daimingzhi Thanks for the testing. But I am not sure about your asking. Is the bootstrapInstrumentation core working for extClassLoader case? I can see you still said there is error happening.

daimingzhi commented 3 years ago

I am so sorry that I uploaded a wrong picture, I have deleted it.

Is the bootstrapInstrumentation core working for extClassLoader case?

Yes,If use bootstrapInstrumentation,everything work well

wu-sheng commented 3 years ago

OK, then we need to get a resolution about how to activate the bootstrap mechanism for extBootstrap, The way you mentioned, Plugin.BootstrapInstrumentation.dirs=apm-sniffer\apm-sdk-plugin\httpClient-4.x-plugin, but, do we need to ask users to set one by one?

daimingzhi commented 3 years ago

but, do we need to ask users to set one by one?

Perhaps, the directory configuration can consider supporting wildcards.like this:

public static class Plugin {
    // default is empty, means by isBootstrapInstrumentation method determines whether a plugin is BootstrapInstrumentation plugin. 
    // If we configure this config to "plugins/*" means all plugin in the plugins directory is BootstrapInstrumentation plugin.  
    // and if we configure this config to "plugins/*jdk*" means all plugins in the plugins directory that contain "jdk" in their name is BootstrapInstrumentation plugin
    // Also, for all plugins not included in this directory, whether it is an BootstrapInstrumentation plugin depends on sBootstrapInstrumentation method
    public static String BOOTSTRAP_INSTRUMENTATION_DIRS = "";
}

wu-sheng commented 3 years ago

This should be not linking to bootstrap, it is just code level sharing, but it is for resolving different issues.

wu-sheng commented 3 years ago

You could consider wildcard, but the configuration should be ext_class_loader related.

wu-sheng commented 3 years ago

@daimingzhi Any update?

daimingzhi commented 3 years ago

I am sorry,it is too busy on Mondays and Tuesdays because of product release. I hava finished code,but still need your help what I did like this :

first,I added a configuration in org.apache.skywalking.apm.agent.core.conf.Config.Plugin

as you said ext_class_loader related,it's named EXT_INSTRUMENTATION_PLUGINS

second,I added a property named isExtInstrumentation and its getter/setter in AbstractClassEnhancePluginDefine

and I did something similar in PluginFinder

third,I use PluginInitializer to initialize a AbstractClassEnhancePluginDefine instance. during this process,If the name of the plugin matches the configuration, the set method will be called and isExtInstrumentation will be set to true

at last,I added in org.apache.skywalking.apm.agent.core.plugin.bootstrap.BootstrapInstrumentBoost#inject

which code like this:

private static void prepareExtInstrumentation(PluginFinder pluginFinder, Map<String, byte[]> classesTypeMap) {
    TypePool typePool = TypePool.Default.of(BootstrapInstrumentBoost.class.getClassLoader());
    // getExtClassMatchDefine is a new method I metationed above
    List<AbstractClassEnhancePluginDefine> extClassMatchDefines = pluginFinder.getExtClassMatchDefine();
    for (AbstractClassEnhancePluginDefine define : extClassMatchDefines) {
        // InstanceMethod use v2Interceptor
        if (Objects.nonNull(define.getInstanceMethodsInterceptV2Points())) {
            for (InstanceMethodsInterceptV2Point point : define.getInstanceMethodsInterceptV2Points()) {
                if (point.isOverrideArgs()) {
                    generateDelegator(classesTypeMap, typePool,
                                      INSTANCE_METHOD_V2_WITH_OVERRIDE_ARGS_DELEGATE_TEMPLATE, point.getMethodsInterceptorV2());
                } else {
                    generateDelegator(classesTypeMap, typePool, INSTANCE_METHOD_V2_DELEGATE_TEMPLATE,
                                      point.getMethodsInterceptorV2());
                }
            }
        }
         // supprot consutrctor interceptor
        if (Objects.nonNull(define.getConstructorsInterceptPoints())) {
            for (ConstructorInterceptPoint point : define.getConstructorsInterceptPoints()) {
                generateDelegator(classesTypeMap, typePool, CONSTRUCTOR_DELEGATE_TEMPLATE,
                                  point.getConstructorInterceptor());
            }
        }

        // staticMethod use v2Interceptor
        if (Objects.nonNull(define.getStaticMethodsInterceptV2Points())) {
            for (StaticMethodsInterceptV2Point point : define.getStaticMethodsInterceptV2Points()) {
                if (point.isOverrideArgs()) {
                    generateDelegator(classesTypeMap, typePool,
                                      STATIC_METHOD_V2_WITH_OVERRIDE_ARGS_DELEGATE_TEMPLATE, point.getMethodsInterceptorV2());
                } else {
                    generateDelegator(classesTypeMap, typePool, STATIC_METHOD_V2_DELEGATE_TEMPLATE,
                                      point.getMethodsInterceptorV2());
                }
            }
        }
    }
}

That's all I did. I do not know if it is acceptable, eager to look forward to your opinion.

wu-sheng commented 3 years ago

Have you tried to directly change the PluginConfig? Add a field, if it exists, don't call the method.

daimingzhi commented 3 years ago

Can you give me more detailed information? I could not catch your meaning precisely. In fact, I am not good at English

Have you tried to directly change the PluginConfig? Add a field, if it exists, don't call the method.

Does this mean I should use reflection other than setter to operate the isExtInstrumentation field that I added?

wu-sheng commented 3 years ago

Oh, sorry, I thought you have go-through all plugin codes. Here are the code level details.

I mean we could enhance PluginBootstrap#loadPlugins, before it returns AbstractClassEnhancePluginDefine list. We could add a new field in AbstractClassEnhancePluginDefine called isExtClassLoaderLoaded(boolean). According to the Config#Plugin#PLUGINS_IN_EXT_CLASS_LOADER, the field value could be set to true(false in default). Then you could control all the logic according to this. And rename the methods according, xxxBootstrapyyy -> xxxBootstrapAndExtyyy, because they should share the same logic, right?

daimingzhi commented 3 years ago

I mean we could enhance PluginBootstrap#loadPlugins, before it returns AbstractClassEnhancePluginDefine list. We could add a new field in AbstractClassEnhancePluginDefine called isExtClassLoaderLoaded(boolean). According to the Config#Plugin#PLUGINS_IN_EXT_CLASS_LOADER, the field value could be set to true(false in default)

That's what I did with the first and second points I mentioned above. And I will rename the configuration to PLUGINS_IN_EXT_CLASS_LOADER and field to isExtClassLoaderLoaded。

I have enhanced loadPlugins method with PluginInitializer in the third point I mentioned

Then you could control all the logic according to this. And rename the methods according, xxxBootstrapyyy -> xxxBootstrapAndExtyyy, because they should share the same logic, right?

I believe I got your idea. When constructing the PluginFinder，I just need to push every ExtClassLoaderLoaded pluginDefine to bootstrapClassAndExtClassMatchDefine list,whose original name is bootstrapClassMatchDefine

Is this right? May I submit a pr for this?

By the way, This was my first time working on an open source project，thank you for your patience!

wu-sheng commented 3 years ago

Is this right? May I submit a pr for this?

Basically, I think we have the consensus. Let's check the details on the PR. It is common we polish more details on the PR.

By the way, This was my first time working on an open source project，thank you for your patience!

I think you are doing great. Pre-discussed is very important to continue the code level deep dive. SkyWalking is many people's first open source PR :) We are glad to guide you in the open source, and hope you could do more out of SkyWalking.

wu-sheng commented 3 years ago

And in the PR, we will ask for a plugin test case like bootstrap plugin, but this time, the app is going to be loaded through ext class loader. Do you know how to write plugin test doc?

Bootstrap case is https://github.com/apache/skywalking/tree/master/test/plugin/scenarios/jdk-http-scenario
Plugin dev doc(test related) is here, https://skywalking.apache.org/docs/main/latest/en/guides/java-plugin-development-guide/#plugin-test-tool

daimingzhi commented 3 years ago

Do you know how to write plugin test doc?

I really don't know,But I will learn about it,Thanks for you reminding.I'll look at other people's PR first. It may take some time, but I'll try as soon as possible

wu-sheng commented 3 years ago

Do you know how to write plugin test doc?

I really don't know,But I will learn about it,Thanks for you reminding.I'll look at other people's PR first. It may take some time, but I'll try as soon as possible

You could submit a pull request w/o the test case at the beginning. Then we could do review and add test simultaneously. That is the good part of open source collaboration.

wu-sheng commented 3 years ago

The final update is here, https://github.com/apache/skywalking-java/pull/63

apache / skywalking