Support Hessian 4.0.x. by wayilau · Pull Request #2644 · apache/skywalking

wayilau · 2019-05-10T02:53:36Z

Please answer these questions before submitting pull request

Why submit this pull request?
Bug fix
New feature provided
Improve performance

Plugin Support hessian 4.0.x.

wu-sheng · 2019-05-10T02:54:56Z

You need to submit test case to here https://github.com/SkyAPMTest/agent-auto-integration-testcases

You could find the document there too.

wayilau · 2019-05-10T02:57:30Z

You need to submit test case to here https://github.com/SkyAPMTest/agent-auto-integration-testcases

You could find the document there too.
ok, i will do this follow the document .

coveralls · 2019-05-10T07:59:04Z

Coverage decreased (-0.0008%) to 16.423% when pulling b9665682c3a2df8b2897fae1198a87eb17f1f53d on wayilau:feature2 into 118485f on apache:master.

coveralls · 2019-05-10T07:59:04Z

Coverage increased (+0.2%) to 17.1% when pulling fcf7b3716fed898c6c6bfebb8e2418a3baaab1b5 on wayilau:feature2 into 4f89ea2 on apache:master.

IanCao · 2019-05-15T02:48:48Z

The ExitSpan isn't been created all the time, but stopspan is always.
Here maybe some logical problems

ok,thanks for your review. i will check.

IanCao · 2019-05-15T02:49:03Z

same as above

wu-sheng · 2019-05-15T12:53:55Z

Please resolve the conflict, the shardingsphere plugin just got merged.

wu-sheng · 2019-05-24T14:16:26Z

Be advised, RESTEasy plugin merged, conflict needs to be solved.

wu-sheng · 2019-05-27T09:00:46Z

Also, as an RPC framework, Hessian logo is required in UI. Take apache/skywalking-rocketbot-ui#113 as an example.

wayilau · 2019-05-28T01:48:07Z

Also, as an RPC framework, Hessian logo is required in UI. Take apache/skywalking-rocketbot-ui#113 as an example.

I will add this after i finished the tests.

wayilau · 2019-06-03T01:30:19Z

@wu-sheng I have add an Instrumentation， but it doesn't works for me, also I can not debug into the Instrmentation when the agent start. so I could not get expected data when i was taking the skyapm tests. do I make some config mistakes ?

wu-sheng · 2019-06-03T01:38:55Z

I don't know, the codes look like normal. Does other plugin work? Agent could be tested, read blog, http://skywalking.apache.org/zh/blog/2019-01-24-skywalking-remote-debug.html (CN) and learn remote debug.

wayilau · 2019-06-03T01:50:16Z

I don't know, the codes look like normal. Does other plugin work? Agent could be tested, read blog, http://skywalking.apache.org/zh/blog/2019-01-24-skywalking-remote-debug.html (CN) and learn remote debug.
I provide 3 instrumentation，the other two works fine. I am finding the reason. the agent Transform the class when it is loading. also this class i intercept is loading, but just not work for me.

wu-sheng · 2019-06-03T01:52:18Z

I provide 3 instrumentation，the other two works fine. I am finding the reason. the agent Transform the class when it is loading. also this class i intercept is loading, but just not work for me.

There is no special case for instrumentation.

wayilau · 2019-06-21T07:16:09Z

@wu-sheng why this builds fails, it tells me that other pkg's class is not allowed.

kezhenxu94 · 2019-06-22T00:31:42Z

@wu-sheng why this builds fails, it tells me that other pkg's class is not allowed.

Yes, importing third parties' class will break the agent when there is no such class in the target application, see #2871 and #2908 , and never try to use full qualified class names to work around the failure, use string literature if needed

asfgit · 2019-06-23T08:53:50Z

FAILURE

asfgit · 2019-06-23T08:53:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/skywalking-e2e/16/

wayilau · 2019-07-22T05:58:29Z

@kezhenxu94 Thanks for many useful suggestions. I have fixed this.

kezhenxu94

Hi @wayilau , I noticed that you are instrumenting class org.springframework.remoting.caucho.HessianServiceExporter, which is not part of hessian core, and we cannot assume that the users use hessian with Spring framework always; actually hessian can be used in plain Java application like what I did based on your agent test code:

add Jetty dependency:

        <dependency>
            <groupId>org.mortbay.jetty</groupId>
            <artifactId>jetty</artifactId>
            <version>6.1.26</version>
        </dependency>

make your HelloServiceImpl extends HessianServlet:
modify the main method of the HessianServer:

@SpringBootApplication
public class HessianServer
{

    @Autowired
    private HelloService helloService = new HelloServiceImpl();

    public static void main( String[] args ) throws Exception {
        final Server server = new Server(9099);
        final Context context = new Context(server, "/", Context.SESSIONS);
        context.addServlet(HelloServiceImpl.class, "/HelloService");
        server.start();
    }

    @Bean(name="/HelloService")
    public HessianServiceExporter getExporter() {
        HessianServiceExporter exporter = new HessianServiceExporter();
        exporter.setServiceInterface(HelloService.class);
        exporter.setService(helloService);
        return exporter;
    }

}

we don't need all the annotations from Spring, but I'm lazy to remove :)

run the server as well as client

And the traces would break like this:

I have no experience with this case and I'm not sure whether this is acceptable, @wu-sheng WDYT

wu-sheng · 2019-07-23T01:35:24Z

I am not a hession user, too. But org.springframework.remoting.caucho.HessianServiceExporter looks like not right indeed. @wayilau Why do you need to choose a Spring framework class? For most Spring components, they are just bridges, a hession-core level instrumentation should be enough. But if there is an exception, please make it clear to us.

Basically, I think @kezhenxu94 get the point, it should work w/ and w/o Spring both like other plugins did.

wayilau · 2019-07-23T01:52:31Z

I am not a hession user, too. But org.springframework.remoting.caucho.HessianServiceExporter looks like not right indeed. @wayilau Why do you need to choose a Spring framework class? For most Spring components, they are just bridges, a hession-core level instrumentation should be enough. But if there is an exception, please make it clear to us.

Basically, I think @kezhenxu94 get the point, it should work w/ and w/o Spring both like other plugins did.

@wu-sheng @kezhenxu94
Yes , I give an spring instrumentation, because last month I give the patch, but the tag is the class name in the server side. you tell me that tags name misunderstood so I supplied a flag when true use the url tag, use class name when it is false. I can not get the url, so use the hessianserviceexporter, as @kezhenxu94 said hessian may use in nomal java application, but they all use Servlet, i think so. what 's you idea ?

wayilau · 2019-07-23T01:54:23Z

I am not a hession user, too. But org.springframework.remoting.caucho.HessianServiceExporter looks like not right indeed. @wayilau Why do you need to choose a Spring framework class? For most Spring components, they are just bridges, a hession-core level instrumentation should be enough. But if there is an exception, please make it clear to us.
Basically, I think @kezhenxu94 get the point, it should work w/ and w/o Spring both like other plugins did.

@wu-sheng @kezhenxu94
Yes , I give an spring instrumentation, because last month I give the patch, but the tag is the class name in the server side. you tell me that tags name misunderstood so I supplied a flag when true use the url tag, use class name when it is false. I can not get the url, so use the hessianserviceexporter, as @kezhenxu94 said hessian may use in nomal java application, but they all use Servlet, i think so. what 's you idea ?

I shoud add an instrumentation for HessianServlet ? am i right ? @kezhenxu94
In springboot it doesn't use HessianServlet, it just uses HessianServiceExporter. so I give two instrumentation, it is enough?

kezhenxu94 · 2019-07-23T04:34:19Z

I shoud add an instrumentation for HessianServlet ? am i right ? @kezhenxu94
In springboot it doesn't use HessianServlet, it just uses HessianServiceExporter. so I give two instrumentation, it is enough?

Spring doesn't use HessianServlet indeed, but it uses HessianSkeleton (org.springframework.remoting.caucho.HessianExporter#doInvoke), HessianServlet also uses HessianSkeleton (com.caucho.hessian.server.HessianServlet#invoke), and it seems that you have already instrumented HessianSkeleton, so perhaps you can instrument HessianSkeleton and use SkyWalking dynamic field (or something similar) to keep the url (if any) in one request through the instrumented classes.

To summarize, I think class HessianSkeleton is the core of hessian, which should be used by any other frameworks that integrate with hessian, such as Spring, and instrumenting it should work with/wihout third-party frameworks

wayilau · 2019-07-24T01:04:01Z

I shoud add an instrumentation for HessianServlet ? am i right ? @kezhenxu94
In springboot it doesn't use HessianServlet, it just uses HessianServiceExporter. so I give two instrumentation, it is enough?

Spring doesn't use HessianServlet indeed, but it uses HessianSkeleton (org.springframework.remoting.caucho.HessianExporter#doInvoke), HessianServlet also uses HessianSkeleton (com.caucho.hessian.server.HessianServlet#invoke), and it seems that you have already instrumented HessianSkeleton, so perhaps you can instrument HessianSkeleton and use SkyWalking dynamic field (or something similar) to keep the url (if any) in one request through the instrumented classes.

To summarize, I think class HessianSkeleton is the core of hessian, which should be used by any other frameworks that integrate with hessian, such as Spring, and instrumenting it should work with/wihout third-party frameworks

Yes, you are right. I will have a try to get the url from the HessianSkeleton and remove the spring's instrumentation.

wayilau · 2019-07-25T08:28:26Z

@kezhenxu94 @wu-sheng
I think it's hard to get url from HessianSkeleton. In HessianSkeleton i only can get the api class. If i want get url, i should send it in the stream from client side, and get it from the inputstream in server side. that's not an easy thing.

wu-sheng · 2019-07-25T08:40:31Z

I think it's hard to get url from HessianSkeleton. In HessianSkeleton i only can get the api class. If i want get url, i should send it in the stream from client side, and get it from the inputstream in server side. that's not an easy thing.

This is server side instrumentation, right? Why send from client side? That header should not be added.

wayilau · 2019-07-25T09:09:16Z

I think it's hard to get url from HessianSkeleton. In HessianSkeleton i only can get the api class. If i want get url, i should send it in the stream from client side, and get it from the inputstream in server side. that's not an easy thing.

This is server side instrumentation, right? Why send from client side? That header should not be added.

Yes, HessianSkeleton is an instrumentation. in HessianSkeleton it only read or write data from stream.
Header value is in HessianServlet or HessianServiceExporter, the are the library out of hessian.

wu-sheng · 2019-07-25T09:11:22Z

Yes, HessianSkeleton is an instrumentation. in HessianSkeleton it only read or write data from stream.
Header value is in HessianServlet or HessianServiceExporter, the are the library out of hessian.

Are you saying the data in HessianSkeleton is only binary and not encoded yet? If so, don't instrument there.

wu-sheng · 2019-07-30T04:07:09Z


+        <dependency>
+            <groupId>org.projectlombok</groupId>
+            <artifactId>lombok</artifactId>


Lomhok has been added at root.

wayilau · 2019-07-31T09:31:39Z

@kezhenxu94 Do you have any time to review this pr ? i have removed the spring's instrumentation.

kezhenxu94 · 2019-07-31T12:03:56Z

@kezhenxu94 Do you have any time to review this pr ? i have removed the spring's instrumentation.

@wayilau Did you test what I mentioned in #2644 (review) ? Seems that the trace still breaks, what's worse, the topology becomes uninterpretable

kezhenxu94 · 2019-08-01T00:44:30Z

Forget to attach the topology screenshot

wayilau · 2019-08-01T01:02:20Z

@kezhenxu94 Do you have any time to review this pr ? i have removed the spring's instrumentation.

@wayilau Did you test what I mentioned in #2644 (review) ? Seems that the trace still breaks, what's worse, the topology becomes uninterpretable

sorry， i didn't do that, i will test this follow your code.

wayilau · 2019-08-08T06:29:13Z

I close this first. i will reopen when i finish this.

wu-sheng added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. TBD To be decided later, need more discussion or input. labels May 10, 2019

IanCao reviewed May 15, 2019

View reviewed changes

wayilau closed this Jun 21, 2019

Fix ci

279ae78

wayilau reopened this Jun 21, 2019

wayilau added 2 commits June 21, 2019 10:04

Add Hessian Config

155f851

Fix config.

8d354f6

wayilau added 3 commits June 21, 2019 15:22

rename plugin.

0d3c35e

Add hessian tags.

fdf28d5

remote unused import

baf4c46

Remove the third classes.

ea19c45

kezhenxu94 reviewed Jul 22, 2019

View reviewed changes

Comment thread ...rg/apache/skywalking/apm/plugin/hessian/v4/define/HessianServiceExporterInstrumentation.java Outdated

Fix the index.

53bffe0

kezhenxu94 reviewed Jul 23, 2019

View reviewed changes

wayilau added 2 commits July 30, 2019 10:04

Remove third party instrumentation.

a30640b

Fix the style.

42eca6b

wu-sheng reviewed Jul 30, 2019

View reviewed changes

wayilau added 6 commits July 30, 2019 14:27

Remove lombok & fix ci.

cceeccb

Fix server side interceptor.

de6ca7e

Merge branch 'master' into feature2

8a79bd9

Fix ci.

e644426

Merge branch 'feature2' of github.com:wayilau/skywalking into feature2

6d04736

Move protecd to public to fix the ci.

c75bb21

Merge branch 'master' into feature2

758110d

wu-sheng added the no update The owner doesn't provide further feedback. label Aug 6, 2019

wayilau closed this Aug 8, 2019

Conversation

wayilau commented May 10, 2019

Uh oh!

wu-sheng commented May 10, 2019

Uh oh!

wayilau commented May 10, 2019

Uh oh!

coveralls commented May 10, 2019

Uh oh!

coveralls commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IanCao May 15, 2019

Choose a reason for hiding this comment

Uh oh!

wayilau May 15, 2019

Choose a reason for hiding this comment

Uh oh!

IanCao May 15, 2019

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented May 15, 2019

Uh oh!

wu-sheng commented May 24, 2019

Uh oh!

wu-sheng commented May 27, 2019

Uh oh!

wayilau commented May 28, 2019

Uh oh!

wayilau commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng commented Jun 3, 2019

Uh oh!

wayilau commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng commented Jun 3, 2019

Uh oh!

wayilau commented Jun 21, 2019

Uh oh!

kezhenxu94 commented Jun 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfgit commented Jun 23, 2019

Uh oh!

asfgit commented Jun 23, 2019

Uh oh!

wayilau commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kezhenxu94 left a comment

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Jul 23, 2019

Uh oh!

wayilau commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wayilau commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kezhenxu94 commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wayilau commented Jul 24, 2019

Uh oh!

wayilau commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng commented Jul 25, 2019

Uh oh!

wayilau commented Jul 25, 2019

Uh oh!

wu-sheng commented Jul 25, 2019

Uh oh!

wu-sheng Jul 30, 2019

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 10, 2019 •

edited

Loading

wayilau commented Jun 3, 2019 •

edited

Loading

wayilau commented Jun 3, 2019 •

edited

Loading

kezhenxu94 commented Jun 22, 2019 •

edited

Loading

wayilau commented Jul 22, 2019 •

edited

Loading

wayilau commented Jul 23, 2019 •

edited

Loading

wayilau commented Jul 23, 2019 •

edited

Loading

kezhenxu94 commented Jul 23, 2019 •

edited

Loading

wayilau commented Jul 25, 2019 •

edited

Loading