Add okhttp2.x plugin by xu1009 · Pull Request #49 · apache/skywalking-java

xu1009 · 2021-10-12T06:17:21Z

Add an agent plugin to support

Add a test case for the new plugin, refer to the doc
[] Add a component id in the component-libraries.yml
Add a logo in the UI repo

If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<7889>.
Update the CHANGES log.

wu-sheng · 2021-10-12T06:47:26Z

Please fix conflicts.

xu1009 · 2021-10-12T07:17:34Z

ok fix conflict

xu1009 · 2021-10-12T08:53:46Z

it looks hbase scenarios test failed, i do not change other file

test/plugin/scenarios/okhttp2-scenario/support-version.list

wu-sheng · 2021-10-12T11:47:51Z

Please resolve conflicts.

xu1009 · 2021-10-12T12:15:29Z

ok, fix it

wu-sheng · 2021-10-12T13:36:27Z

Your #45 gets merged, you have to resolve the conflicts again.

xu1009 · 2021-10-12T14:12:51Z

ok, fix conflicts

...2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/okhttp/v2/RealCallInterceptor.java

wu-sheng · 2021-10-13T02:37:10Z

test/plugin/scenarios/okhttp2-scenario/config/expectedData.yaml

+# limitations under the License.
+segmentItems:
+- serviceName: okhttp2-scenario
+  segmentSize: ge 5


Why ge 5?

at least one health check

We don't have to do this. Usually ge 0 is clear enough.

wu-sheng · 2021-10-13T02:37:37Z

test/plugin/scenarios/okhttp2-scenario/config/expectedData.yaml

+  - segmentId: not null
+    spans:
+    - operationName: Async/okhttp2-scenario/case/receiveContext-0
+      parentSpanId: 0
+      spanId: 1
+      startTime: nq 0
+      endTime: nq 0
+      componentId: 0
+      isError: false
+      spanType: Local
+      skipAnalysis: 'false'
+    - operationName: GET:/case/okhttp2-scenario
+      parentSpanId: -1
+      spanId: 0
+      spanLayer: Http
+      startTime: nq 0
+      endTime: nq 0
+      componentId: 14
+      isError: false
+      spanType: Entry
+      tags:
+      - {key: url, value: 'http://localhost:8080/okhttp2-scenario/case/okhttp2-scenario'}
+      - {key: http.method, value: GET}
+      skipAnalysis: 'false'


What is this segment? It seems no exit and no ref?

I assume this is the main entry and the next should be async call, right?

I can see /okhttp2-scenario/case/receiveContext-0 exit span, but where is /okhttp2-scenario/case/receiveContext-1 exit span?

yes the segment next is async call, it follows the passed okhttp version

/okhttp2-scenario/case/receiveContext-1 exit span seems lost, i recheck it

i find the problem, the request is in async call back respose method, but the last span is exit span, this causes this span will be discarded

it looks the passed okhttp version has the same problem, besides, the time of action in response method will be calculated in last span, it is not correct

This span will be ignore only if both spans are in the same thread. But callback usually should generate a local span. Also, callback should invoke after the first HTTP span finished, right?

yes, both span is in same thread, the implementation is strange, i will optimize it

That means the first exit span doesn't finish in time.

wu-sheng · 2021-10-13T02:39:31Z

...rio/src/main/java/test/apache/skywalking/apm/testcase/okhttp2/controller/CaseController.java

+        Request request = new Request.Builder().url("http://127.0.0.1:8080/okhttp2-scenario/case/receiveContext-0")
+                .header("sw8", "123456").build();


Could you explain a little about this? sw8 header is not in this format.
When you are doing request, the header should be injected automatically. Is this somthing special?

it can confirm the plugin overwrite the skywalking sw8 head

Why do we need to test this scenario? Who will use this header key besides ourselves?

ok, i will remove it

i fix it by add local span in response, so it can catch the action in reposne method and the first http call time is correct like above

Local span should be added.

A question is, from the graph, the duration of first span is incorrect. It should finish before the local span starts, otherwise, the rpc duration is over measured.

yes, but self time cost is the duration of first span,

That is correct. But when you look at metrics of lines in the topology, which represents calling outbound to upstream, the number of client side is generated by exit span.
So, we also need to fix that duration too.

ok i will fix it and the passed okhttp version too

wu-sheng · 2021-10-19T08:12:19Z

@xu1009 Could you finish this pull request?

xu1009 · 2021-10-19T15:31:36Z

@xu1009 Could you finish this pull request?

@wu-sheng sorry about it, i am very busy recently, i will try my best to finish it this week

wu-sheng · 2021-10-23T00:58:37Z

New release should start recently, if you want to include this in, you need to finish this PR ASAP.

xu1009 · 2021-10-25T06:40:20Z

i fix it like above, you think ok

execute is local span，include execute http req and response callback fail callback

wu-sheng · 2021-10-25T06:43:31Z

This trace seems good now. Is there other version(s) to fix?

xu1009 · 2021-10-25T06:49:51Z

no, i think it is the best, it can show the time cost precisely

wu-sheng · 2021-10-25T06:51:12Z

no, i think it is the best, it can show the time cost precisely

I just mean this, there are several versions to fix, right?
#49

xu1009 · 2021-10-25T06:52:28Z

yes, i ci it in this way

…khttp2.x-plugin

CHANGES.md

wu-sheng · 2021-10-25T11:15:17Z

Please fix CI.

xu1009 · 2021-10-25T11:34:48Z

ok, okhttp4.x use kt, it looks internal method name is diff in running time

xu1009 · 2021-10-25T12:04:37Z

the failed ci task is not my change

wu-sheng · 2021-10-25T12:11:28Z

the failed ci task is not my change

I rerun those. But it seems you missed some UTs?

xu1009 · 2021-10-25T12:18:24Z

yes, i find the problem, fix it

wu-sheng · 2021-10-25T14:04:04Z

Strange, that plugin can't pass even I have tried many times.

xu1009 · 2021-10-25T14:16:10Z

i do not change that plugin==

xu1009 · 2021-10-25T14:21:53Z

i will try the master branch

xu1009 · 2021-10-25T14:36:25Z

the master branch failed too

wu-sheng · 2021-10-25T14:37:06Z

i do not change that plugin==

I know you didn't change that.

* main: (28 commits) fix release doc (#61) Support Jedis' Transaction and fix duplicated enhancement (#57) Initialize 8.9.0 iteration (#59) Polish release shell and doc. (#58) fix rocketmq message header properties garbled characters issue (#54) Fix netty-socketio plugin test failure (#56) Add okhttp2.x plugin (#49) add e2e test for kafka transporter (#42) Fix version badge (#53) Add JDK17 supported declaration. (#52) Fix instrumentation v2 API doesn't work for constructor instrumentation. (#51) Add kylin jdbc plugin (#45) Fix version compatibility for JSON-RPC4J Plugin (#50) Feature add clickhouse jdbc plugin (#41) Update menu.yml (#48) Fix format. (#47) Add new menu for the document (#46) Doc: Update setup agent in kubernetes from 'containers' to 'initContainers'. (#44) The httpasyncclient-4.x-plugin does not take effect every time. (#40) Add an agent plugin to support Jackson (#39) ... # Conflicts: # .github/workflows/plugins-test.3.yaml # CHANGES.md # docs/en/setup/service-agent/java-agent/README.md # docs/menu.yml

litexu added 3 commits October 11, 2021 16:20

support okhttp 2.x

26e74d9

update scenario

449ec3d

change packge

31f4803

wu-sheng added enhancement New feature or request plugin labels Oct 12, 2021

fix conflicts

604e0f1

wu-sheng reviewed Oct 12, 2021

View reviewed changes

test/plugin/scenarios/okhttp2-scenario/support-version.list Outdated Show resolved Hide resolved

remove some patch version

d36304a

wu-sheng added this to the 8.8.0 milestone Oct 12, 2021

merge master and fix and add git hub action

6be6cbc

merge master and fix conflicts

0a3c8ba

wu-sheng reviewed Oct 12, 2021

View reviewed changes

...2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/okhttp/v2/RealCallInterceptor.java Outdated Show resolved Hide resolved

optimize code style

b248f11

wu-sheng reviewed Oct 13, 2021

View reviewed changes

Merge branch 'main' into add-okhttp2.x-plugin

c52276a

litexu added 2 commits October 25, 2021 17:10

optimize okhttp plugin

48a871c

Merge remote-tracking branch 'origin/add-okhttp2.x-plugin' into add-o…

092ef9a

…khttp2.x-plugin

wu-sheng reviewed Oct 25, 2021

View reviewed changes

CHANGES.md Show resolved Hide resolved

litexu added 2 commits October 25, 2021 17:16

add change log

ccf63c7

fix code style

516cbb5

fix okhttp 4.x bug

a253c47

fix ci

683a7d0

wu-sheng approved these changes Oct 25, 2021

View reviewed changes

wu-sheng merged commit 9101f69 into apache:main Oct 25, 2021

wu-sheng mentioned this pull request Oct 25, 2021

[Bug] Netty socketio CI failing apache/skywalking#8001

Closed

3 tasks

mrproliu mentioned this pull request Oct 26, 2021

Fix netty-socketio plugin test failure #56

Merged

2 tasks

wu-sheng mentioned this pull request Nov 5, 2021

[Feature] Add okhttp 2.x plugin apache/skywalking#7889

Closed

3 tasks

GuoHaoZai pushed a commit to GuoHaoZai/skywalking-java that referenced this pull request Apr 24, 2025

Rename go package name (apache#49)

743f331

		Request request = new Request.Builder().url("http://127.0.0.1:8080/okhttp2-scenario/case/receiveContext-0")
		.header("sw8", "123456").build();

Conversation

xu1009 commented Oct 12, 2021

Add an agent plugin to support

Uh oh!

wu-sheng commented Oct 12, 2021

Uh oh!

xu1009 commented Oct 12, 2021

Uh oh!

xu1009 commented Oct 12, 2021

Uh oh!

Uh oh!

wu-sheng commented Oct 12, 2021

Uh oh!

xu1009 commented Oct 12, 2021

Uh oh!

wu-sheng commented Oct 12, 2021

Uh oh!

xu1009 commented Oct 12, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu1009 Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Oct 19, 2021

Uh oh!

xu1009 commented Oct 19, 2021

Uh oh!

wu-sheng commented Oct 23, 2021

Uh oh!

xu1009 commented Oct 25, 2021

Uh oh!

wu-sheng commented Oct 25, 2021

Uh oh!

xu1009 commented Oct 25, 2021

Uh oh!

xu1009 Oct 14, 2021 •

edited

Loading