Set up the bootstrap instrumentation plugin framework #3152

wu-sheng · 2019-07-23T15:02:12Z

Hi, All

This PR is following #2932, in these days, I work on building the bootstrap instrumentation plugin framework by not breaking any existing plugin. Look like it works.

For plugin developer, one line required only in plugin define.

     // This is the only change for plugin developer
    @Override public boolean isBootstrapInstrumentation() {
        return true;
    }

wu-sheng · 2019-07-23T15:09:00Z

If you want to understand how I do this, follow BootstrapInstrumentBoost class, it will link the things together. Also, plugin interceptor point definitions have been as public, as BootstrapInstrumentBoost needs to access it.

Also, I add a demo jre-HttpUrlConnection plugin for demo, and instrumen the following application codes.

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.HttpURLConnection;
import java.net.URL;

public class Main {
    public static void main(String[] args) throws IOException {
        URL obj = new URL("http://www.baidu.com");
        HttpURLConnection con = (HttpURLConnection)obj.openConnection();

        con.setRequestMethod("GET");

        int responseCode = con.getResponseCode();
        System.out.println("Response Code : " + responseCode);

        BufferedReader in = new BufferedReader(
            new InputStreamReader(con.getInputStream()));
        String inputLine;
        StringBuffer response = new StringBuffer();

        while ((inputLine = in.readLine()) != null) {
            response.append(inputLine);
        }
        in.close();

        //print result
        System.out.println(response.toString());
    }
}

Like the existing plugins, the bootstrap intrumentation only requires you to declare it in plugin define, such as following codes I included in this PR

/**
 * @author wusheng
 */
public class HttpUrlConnectionInstrumentation extends ClassEnhancePluginDefine {
    private static String CLASS_NAME = "java.net.HttpURLConnection";

     // This is the only change for plugin developer
    @Override public boolean isBootstrapInstrumentation() {
        return true;
    }

    @Override protected ClassMatch enhanceClass() {
        return byName(CLASS_NAME);
    }

    @Override public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
        return new InstanceMethodsInterceptPoint[] {
            new InstanceMethodsInterceptPoint() {
                @Override public ElementMatcher<MethodDescription> getMethodsMatcher() {
                    return named("setRequestMethod");
                }

                @Override public String getMethodsInterceptor() {
                    return "org.apache.skywalking.apm.plugin.jre.httpurlconnection.Interceptor";
                }

                @Override public boolean isOverrideArgs() {
                    return false;
                }
            }
        };
    }
}

Also, I haven't finished construct and static method plugin, but they are very similar, as we have this, they are simple. I will push those later.

I hope you could test this new mechanism in other demo codes, Spring Boot and Tomcat especially, in case I miss anything.

@ascrutae Please lead the tests.

…pports JDK 1.6 compile, according to https://github.com/jboss-javassist/javassist/blob/rel_3_23_2_ga/pom.xml#L149-L160

wu-sheng · 2019-07-24T09:28:43Z

This PR is ready to review and test. I have used it to instrument URL and HttpURLConnection successfully. But just a demo code, of course.

Part of my test codes are posted here, https://gist.github.com/wu-sheng/25a8877971db089e196ada0fe4395813

In this PR, there is no real plugin. But further pull request is welcome.

candyleer · 2019-07-24T10:15:29Z

I have tested in bootstrap class and 3rd part class,both work well.
I have a question,the classes in HIGH_PRIORITY_CLASSES still will be loaded by two classloader,right?

but I debug found, eg.EnhancedInstance is loaded by bootstrap classloader in bootstrap interceptor (generated by InstanceMethodInterTemplate),and in normal InstMethodsInter it is loaded by application classloader .is it right?

I think EnhancedInstance will be find in bootstrap classloader first .

wu-sheng · 2019-07-24T12:43:48Z

I have tested in bootstrap class and 3rd part class,both work well.
I have a question,the classes in HIGH_PRIORITY_CLASSES still will be loaded by two classloader,right?
but I debug found, eg.EnhancedInstance is loaded by bootstrap classloader in bootstrap interceptor (generated by InstanceMethodInterTemplate),and in normal InstMethodsInter it is loaded by application classloader .is it right?
I think EnhancedInstance will be find in bootstrap classloader first .

You do the tests quickly :) Thanks.
To your question, from my understanding, when bootstrap instrumentation actived, EnhancedInstance should only be loaded in Bootstrap classloader. Otherwise, you will face Class Cast Exception, because you can't cat Bootstrap's EnhancedInstance to ApplicationClassLoader's EnhancedInstance.

Load the generated class to class loader w/o initialization should be safe. That is why I use #injectRaw rather than #inject.

This is my guess and understanding from test results, I used to try #inject, which cause that cast exception. :)

wu-sheng · 2019-07-24T12:57:48Z

@candyleer If you are OK, please approve this PR.

kezhenxu94 · 2019-07-24T13:02:16Z

@candyleer If you are OK, please approve this PR.

is this file (debug/org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.bootstrap.Interceptor_internal.class) committed by mistake?

wu-sheng · 2019-07-24T13:02:51Z

is this file (debug/org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.bootstrap.Interceptor_internal.class) committed by mistake?

Yes. Removing it.

wu-sheng · 2019-07-24T13:15:16Z

@candyleer Have you tested Tomcat? And JDK 6?

wu-sheng · 2019-07-24T14:55:56Z

/run ci

candyleer · 2019-07-24T16:28:39Z

@candyleer Have you tested Tomcat? And JDK 6?

I have test Tomcat .but JDK 6 not test ,maybe tomorrow I can run a test manually.

candyleer · 2019-07-25T02:19:27Z

It seems not support JDK6 for the error,looks like javassist dependent on some feature above jdk7

wu-sheng · 2019-07-25T02:21:06Z

Look like javassist has an error.

candyleer · 2019-07-25T02:40:57Z

I have tested ,javassist v3.21.0-GA is the last version support jdk6,and we can work well by using it

candyleer · 2019-07-25T02:53:24Z

...org/apache/skywalking/apm/agent/core/plugin/bootstrap/template/ConstructorInterTemplate.java

+        @AllArguments Object[] allArguments) {
+        try {
+            EnhancedInstance targetObject = (EnhancedInstance)obj;
+


should call prepare()

candyleer · 2019-07-25T02:55:35Z

apm-sniffer/apm-agent-core/pom.xml

@@ -37,6 +38,7 @@
        <grpc.version>1.14.0</grpc.version>
        <guava.version>20.0</guava.version>
        <bytebuddy.version>1.9.2</bytebuddy.version>
+        <javaassist.version>3.23.2-GA</javaassist.version>


3.21.0-GA support JDK6

OK. I am doing downgrade

candyleer · 2019-07-25T02:58:26Z

...org/apache/skywalking/apm/agent/core/plugin/bootstrap/template/ConstructorInterTemplate.java

+     * Prepare the context. Link to the agent core in AppClassLoader.
+     */
+    private static void prepare() {
+        if (INTERCEPTOR == null) {


the INTERCPETOR Initialization shoud be synchronized?

Don't' have to. Create multiple instances is still safe. This could just avoid creating too many instances, and also avoid the lock.

wu-sheng · 2019-07-25T05:16:39Z

@candyleer Updated.

wu-sheng · 2019-07-25T05:38:53Z

@ascrutae Please run our autotests, let's see whether this breaks anything.

candyleer

BootstrapPluginLogBridge method not implemented

candyleer · 2019-07-25T05:42:21Z

...ain/java/org/apache/skywalking/apm/agent/core/plugin/bootstrap/BootstrapPluginLogBridge.java

+ *
+ * @author wusheng
+ */
+public class BootstrapPluginLogBridge implements IBootstrapLog {


the method in this class should be implemented, or the log will be NoopLog.

wu-sheng · 2019-07-25T05:49:34Z

...ain/java/org/apache/skywalking/apm/agent/core/plugin/bootstrap/BootstrapPluginLogBridge.java

+    }
+
+    @Override public void info(String format, Object... arguments) {
+        logger.info(format, arguments);


@candyleer Could you test this log method? I am little concerned about arguments(Object...) could work or not. I haven't the test codes at my hand now.

You could simple call log("", arg1, arg2) in InstanceMethodInterTemplate, and see whether log works well?

OK ,I just tested info(String format)

Make sure you use args list, no matter which method you test.

Test passed

wu-sheng · 2019-07-25T12:25:18Z

/run ci

wu-sheng · 2019-07-25T12:53:29Z

/run ci

wu-sheng · 2019-07-25T12:57:14Z

From the test report, it is same as before. After CI pass, I will merge this to the core. The new plugin is welcome, especially for HttpUrlConnection.

wu-sheng added 4 commits July 22, 2019 20:03

POC: Intercept successfully.

0c96c59

Refactor codes

dffc9e3

Build the new core

7ea12f5

Setup the new core.

96f17f1

wu-sheng added TBD To be decided later, need more discussion or input. core feature Core and important feature. Sometimes, break backwards compatibility. agent Language agent related. labels Jul 23, 2019

wu-sheng added this to the 6.3.0 milestone Jul 23, 2019

wu-sheng requested review from zhaoyuguang, candyleer, ascrutae, JaredTan95, kezhenxu94, peng-yongsheng and IanCao July 23, 2019 15:02

wu-sheng mentioned this pull request Jul 23, 2019

[Question] Is redefine suitable for new class generated, which is used as a bootstrap class interceptor? raphw/byte-buddy#689

Closed

wu-sheng added 5 commits July 24, 2019 11:35

Push the javassist version to 3.23.2-GA, as it is the last version su…

340785c

…pports JDK 1.6 compile, according to https://github.com/jboss-javassist/javassist/blob/rel_3_23_2_ga/pom.xml#L149-L160

Merge branch 'master' into bootstrap-cl

7f2e39f

Finish the all codes.

e6d521b

Document updated for bootstrap instrumentation.

0b44556

Merge branch 'master' into bootstrap-cl

c35ec55

wu-sheng changed the title ~~[Don't merge] Set up the bootstrap instrumentation plugin framework~~ Set up the bootstrap instrumentation plugin framework Jul 24, 2019

Add a notice to document.

49cc08e

wu-sheng removed the TBD To be decided later, need more discussion or input. label Jul 24, 2019

Merge branch 'master' into bootstrap-cl

c5923e9

Remove unexpected file.

3163695

Merge branch 'master' into bootstrap-cl

0ee6037

candyleer reviewed Jul 25, 2019

View reviewed changes

Set javassist version right. Add judge in core interceptors

c1a7bef

candyleer reviewed Jul 25, 2019

View reviewed changes

Fixe no log bridge implementation

5e793d7

wu-sheng commented Jul 25, 2019

View reviewed changes

candyleer approved these changes Jul 25, 2019

View reviewed changes

Merge branch 'master' into bootstrap-cl

0b5ef3f

ascrutae approved these changes Jul 25, 2019

View reviewed changes

Merge branch 'master' into bootstrap-cl

52062b7

wu-sheng merged commit c76fb40 into master Jul 25, 2019

wu-sheng deleted the bootstrap-cl branch July 25, 2019 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up the bootstrap instrumentation plugin framework #3152

Set up the bootstrap instrumentation plugin framework #3152

wu-sheng commented Jul 23, 2019 •

edited

wu-sheng commented Jul 23, 2019

wu-sheng commented Jul 24, 2019

candyleer commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

kezhenxu94 commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

candyleer commented Jul 24, 2019

candyleer commented Jul 25, 2019 •

edited

wu-sheng commented Jul 25, 2019

candyleer commented Jul 25, 2019

candyleer Jul 25, 2019

wu-sheng Jul 25, 2019

candyleer Jul 25, 2019

wu-sheng Jul 25, 2019

candyleer Jul 25, 2019

wu-sheng Jul 25, 2019

candyleer Jul 25, 2019

wu-sheng commented Jul 25, 2019

wu-sheng commented Jul 25, 2019

candyleer left a comment

candyleer Jul 25, 2019

wu-sheng Jul 25, 2019

wu-sheng Jul 25, 2019

wu-sheng Jul 25, 2019

candyleer Jul 25, 2019

wu-sheng Jul 25, 2019

candyleer Jul 25, 2019

wu-sheng commented Jul 25, 2019

wu-sheng commented Jul 25, 2019

wu-sheng commented Jul 25, 2019

Set up the bootstrap instrumentation plugin framework #3152

Set up the bootstrap instrumentation plugin framework #3152

Conversation

wu-sheng commented Jul 23, 2019 • edited

wu-sheng commented Jul 23, 2019

wu-sheng commented Jul 24, 2019

candyleer commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

kezhenxu94 commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

wu-sheng commented Jul 24, 2019

candyleer commented Jul 24, 2019

candyleer commented Jul 25, 2019 • edited

wu-sheng commented Jul 25, 2019

candyleer commented Jul 25, 2019

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

wu-sheng commented Jul 25, 2019

wu-sheng commented Jul 25, 2019

candyleer 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

wu-sheng commented Jul 25, 2019

wu-sheng commented Jul 25, 2019

wu-sheng commented Jul 25, 2019

wu-sheng commented Jul 23, 2019 •

edited

candyleer commented Jul 25, 2019 •

edited