Add pulsar apm plugin #3476

codelipenghui · 2019-09-17T10:23:16Z

Please answer these questions before submitting pull request

Why submit this pull request?
Bug fix
[x ] New feature provided
Improve performance
Related issues
Provide plugin for Apache Pulsar #2764

New feature or improvement

Describe the details and related test reports.

Add pulsar apm plugin and unit tests, integration test will add soon.

Local tests result:

As the image shows, created 1 producer and 3 consumers with different subscription name(test-1, test-2, test-3).

Local integration test result:

[INFO] ---------------------------Here is the test report context -------------------------------------

Test Report - 2019-09-24 11:30

tests : 7 passed. 0 failed
branch name : pulsar-plugin
commit id : 88e1ef3eeb2cabc30690c5a42a7f1604e9eff2e7
cases branch: pulsar-scenario
cases commit id: a5731397356122ee0868a9b107f7ded28e46dd03

Cases List

Case description	Status	Cases
Pulsar	7 passed. 0 failed	click me

Pulsar

Version	Passed	Failed
2.3.1	✔️
2.3.0	✔️
2.4.0	✔️
2.4.1	✔️
2.3.2	✔️
2.2.1	✔️
2.2.0	✔️

wu-sheng · 2019-09-17T11:26:20Z

@dmsolr Feel free to join.

wu-sheng · 2019-09-18T14:45:32Z

W/ light4j plugin merged, you need to update the component definition because the ID has been used.

wu-sheng · 2019-09-19T00:16:37Z

After you fix the conflicts and pass tests locally, please post your local test report here.

codelipenghui · 2019-09-19T03:20:12Z

@wu-sheng I have fix the conflicts and past the local test screenshot to the description of this PR, please help take a look the test result screenshot.

dmsolr · 2019-09-19T07:16:01Z

...-plugin/src/main/java/org/apache/skywalking/apm/plugin/pulsar/PulsarConsumerInterceptor.java

+    @Override
+    public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
+                                      Class<?>[] argumentsTypes, Throwable t) {
+        ContextManager.activeSpan().errorOccurred().log(t);


I notice that Span starts after the method, so we will meet NPE when an exception happens before the Span created.

I think we should move create active span logic to the beforeMethod, by the way, shall we need to catch the IllegalStateException here, because if no active span here will throw the IllegalStateException and i can't find a public method to check activeSpanStack is or not empty.

I think you dose not get my points.

In usually, We need not catch the IllegalStateException and need not check the size of the stack. But we have to make sure that the started and stopped of Span must present in pairs in an interceptor. In other word, which starts a Span must stop it.

Usually, We suggest doing like below.

if (ContextManager.isActive()) { ContextManager.activeSpan().errorOccurred().log(t); }

The key point, we have to recheck by the same conditions as a span start when we wanna stop the span.

dmsolr · 2019-09-19T07:16:06Z

...-plugin/src/main/java/org/apache/skywalking/apm/plugin/pulsar/PulsarProducerInterceptor.java

+    @Override
+    public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+                              Object ret) throws Throwable {
+        ContextManager.stopSpan();


I think we need to ensure SPAN has been created will be better.

As mentioned above, to catch the IllegalStateException or is their any method to check the span is created

I think if logically you could generate the span created in Pulsar plugin, it is OK to just stop span directly.

Because even we do isActive check, you could just check the span created by other plugins.

dmsolr · 2019-09-19T07:28:32Z

...ar-plugin/src/main/java/org/apache/skywalking/apm/plugin/pulsar/SendCallbackInterceptor.java

+                             MethodInterceptResult result) throws Throwable {
+        SendCallbackEnhanceRequiredInfo requiredInfo = (SendCallbackEnhanceRequiredInfo) objInst.getSkyWalkingDynamicField();
+        if (null != requiredInfo.getContextSnapshot()) {
+            AbstractSpan activeSpan = ContextManager.createLocalSpan(OPERATION_NAME);


I'm not familiar with Pulsar. In my experience, the LocalSpan can not always refer to ExitSpan, or it will miss directly sometimes.

If the ExitSpan is closed and LocalSpan is still working, it may happen.

In pulsar, i think the span of send callback is always a LocalSpan because in the exit span can guarantee that the callback is complete or complete with exception.

Here is the source of pulsar producer: https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L294

So, If I don't get it wrong, the SendCallback should create a LocalSpan. If there are any mistakes, please point them out.

I got it. But I am thinking of another thing. In a scenario, SendCallBack.sendComplete is block till its parent span closed. At this moment, we call the ContextManager.continued(requiredInfo.getContextSnapshot()) maybe throw an exception.
Maybe we can use Asyn API.(I am not sure.)

I am missing the context here. Could you show me the thread workflow for your concern?

…thod

codelipenghui · 2019-09-19T10:47:32Z

@dmsolr Thanks for your review, i have move the span creation of consumer to beforeMethod. Please help take a look again.

wu-sheng · 2019-09-23T09:08:06Z

@codelipenghui Look like the CI and e2e failure caused by limited resource, es image can't be started in time. I will check this later today. You could focus on your local integration tests only for now.

codelipenghui · 2019-09-23T09:14:33Z

@wu-sheng Ok, thanks. I'm doing the local integration tests now.

...etwork/src/main/java/org/apache/skywalking/apm/network/trace/component/ComponentsDefine.java

...-plugin/src/main/java/org/apache/skywalking/apm/plugin/pulsar/PulsarConsumerInterceptor.java

codelipenghui · 2019-09-24T03:35:51Z

@wu-sheng I have paste the local integration test result to the description of this PR. Please help review the pulsar scenario integration.

wu-sheng · 2019-09-24T07:23:18Z

@ascrutae Please run tests on our INFRA

SkyWalkingRobot · 2019-09-24T08:56:02Z

Here is the test report and validate logs

wu-sheng · 2019-09-24T08:57:25Z

@codelipenghui Tests are 1 passed, 5 failure. Please check the log, we are running the cases parallel, is that some static port in your case?

codelipenghui · 2019-09-24T09:25:42Z

@wu-sheng Currently, pulsar broker using static port, is that the error cause?

wu-sheng · 2019-09-24T09:33:40Z

Currently, pulsar broker using static port, is that the error cause?

You should bind port by using docker compose. This is the cause.

codelipenghui · 2019-09-24T09:41:41Z

Sorry, i'm lost here. I have bind port by using:

expose:
      - "8082"
ports:
      - "{SERVER_OUTPUT_PORT}:8082"

Pulsar broker and pulsar scenario is in one container, why pulsar broker also need to bind a port?

SkyWalkingRobot · 2019-09-24T15:46:49Z

Here is the test report and validate logs

wu-sheng

LGTM, including positive test results.

@dmsolr @kezhenxu94 Please recheck anf confirm

kezhenxu94

LGTM

dmsolr · 2019-09-25T05:22:52Z

LGTM

wu-sheng · 2019-09-25T05:24:46Z

Merging

codelipenghui · 2019-09-25T06:00:29Z

@wu-sheng @dmsolr @kezhenxu94 Thanks for your help, very nice experience in Skywalking community.

wu-sheng · 2019-09-25T06:08:33Z

Welcome and look forward your storage proposal.

wu-sheng · 2019-12-02T02:44:56Z

@codelipenghui The plugin test immigration is on going in these two months, a lot of the plugins have moved into PR related test process. #3583

Could any of you move the pulsar plugin test into the main repo too? The original plugin test env has been shut down. Moving into the main repo could make the pulsar plugin checked in every PR.

codelipenghui · 2019-12-02T02:53:03Z

@wu-sheng Thanks for your reminder, will move the pulsar plugin test soon.

Add pulsar apm plugin.

91ebca5

kezhenxu94 added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Sep 17, 2019

codelipenghui and others added 2 commits September 17, 2019 18:31

Fix check style

5b6e792

Merge branch 'master' into pulsar-plugin

e562e51

wu-sheng requested review from IanCao, kezhenxu94 and zhaoyuguang September 17, 2019 11:26

codelipenghui added 2 commits September 18, 2019 10:01

Fix pulsar consumer component define.

76e9b8e

Merge remote-tracking branch 'origin/pulsar-plugin' into pulsar-plugin

2d09d59

codelipenghui mentioned this pull request Sep 18, 2019

Add pulsar-scenario SkyAPMTest/agent-auto-integration-testcases#90

Merged

codelipenghui changed the title ~~[WIP]Add pulsar apm plugin~~ Add pulsar apm plugin Sep 18, 2019

Merge branch 'master' into pulsar-plugin

3167425

codelipenghui added 3 commits September 19, 2019 10:07

Add pulsar to component-libraries.yml

44bb61d

Merge branch 'master' into pulsar-plugin

a8bda15

Fix error interceptor class.

0222726

codelipenghui and others added 4 commits September 19, 2019 11:36

Add pulsar to agent support list.

bf2e90a

Merge branch 'master' into pulsar-plugin

6e8c09f

Add Pulsar to ComponentsDefine and component-libraries.yml

ec7b752

Merge remote-tracking branch 'origin/pulsar-plugin' into pulsar-plugin

144d639

dmsolr reviewed Sep 19, 2019

View reviewed changes

codelipenghui added 2 commits September 19, 2019 18:30

Move create entry span log of consumer from after method to before me…

67383b9

…thod

Fix send callback issue when exception cause.

88a4a1d

Fix test issues

ddc3b9e

Merge branch 'master' into pulsar-plugin

a217106

dmsolr reviewed Sep 23, 2019

View reviewed changes

...etwork/src/main/java/org/apache/skywalking/apm/network/trace/component/ComponentsDefine.java Show resolved Hide resolved

Merge branch 'master' into pulsar-plugin

53f2e76

dmsolr reviewed Sep 23, 2019

View reviewed changes

...-plugin/src/main/java/org/apache/skywalking/apm/plugin/pulsar/PulsarConsumerInterceptor.java Outdated Show resolved Hide resolved

wu-sheng and others added 4 commits September 24, 2019 09:42

Merge branch 'master' into pulsar-plugin

e0eb229

Merge branch 'master' into pulsar-plugin

11e3bb1

Fix comments

69bba9b

Merge remote-tracking branch 'origin/pulsar-plugin' into pulsar-plugin

88e1ef3

Merge branch 'master' into pulsar-plugin

2d18940

wu-sheng approved these changes Sep 24, 2019

View reviewed changes

kezhenxu94 approved these changes Sep 25, 2019

View reviewed changes

wu-sheng added this to the 6.5.0 milestone Sep 25, 2019

wu-sheng merged commit a874251 into apache:master Sep 25, 2019

codelipenghui deleted the pulsar-plugin branch September 25, 2019 06:00

Add pulsar apm plugin #3476

Add pulsar apm plugin #3476

Conversation

codelipenghui commented Sep 17, 2019 • edited Loading

New feature or improvement

Local tests result:

Local integration test result:

Test Report - 2019-09-24 11:30

Cases List

Pulsar

wu-sheng commented Sep 17, 2019

wu-sheng commented Sep 18, 2019

wu-sheng commented Sep 19, 2019

codelipenghui commented Sep 19, 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

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

codelipenghui commented Sep 19, 2019

wu-sheng commented Sep 23, 2019

codelipenghui commented Sep 23, 2019

codelipenghui commented Sep 24, 2019

wu-sheng commented Sep 24, 2019

SkyWalkingRobot commented Sep 24, 2019

wu-sheng commented Sep 24, 2019

codelipenghui commented Sep 24, 2019

wu-sheng commented Sep 24, 2019

codelipenghui commented Sep 24, 2019 • edited Loading

SkyWalkingRobot commented Sep 24, 2019

wu-sheng left a comment

Choose a reason for hiding this comment

kezhenxu94 left a comment

Choose a reason for hiding this comment

dmsolr commented Sep 25, 2019

wu-sheng commented Sep 25, 2019

codelipenghui commented Sep 25, 2019

wu-sheng commented Sep 25, 2019

wu-sheng commented Dec 2, 2019

codelipenghui commented Dec 2, 2019

codelipenghui commented Sep 17, 2019 •

edited

Loading

codelipenghui commented Sep 24, 2019 •

edited

Loading