Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature: jsonrpc4j plugin #6743

Merged
merged 24 commits into from
Apr 17, 2021
Merged

feature: jsonrpc4j plugin #6743

merged 24 commits into from
Apr 17, 2021

Conversation

gy09535
Copy link
Contributor

@gy09535 gy09535 commented Apr 13, 2021 •
edited

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

close: #6742

@kezhenxu94 kezhenxu94 added agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Apr 13, 2021
wu-sheng
wu-sheng reviewed Apr 13, 2021
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the plugin contribution document, we require the integration plugin test.
And as this is a personal project, I need to do some research whether we should accept this as an official plugin.

kezhenxu94
kezhenxu94 reviewed Apr 13, 2021
View reviewed changes
...ffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/tag/Tags.java Outdated
@@ -102,6 +102,8 @@ private Tags() {

public static final String VAL_LOCAL_SPAN_AS_LOGIC_ENDPOINT = "{\"logic-span\":true}";

public static final StringTag JSON_RPC_METHOD = new StringTag(1, "jsonrpc.method");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually only put general tags into apm-agent-core module, maybe you should consider moving this tag into a Constants util class in the jsonrpc4j module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually only put general tags into apm-agent-core module, maybe you should consider moving this tag into a Constants util class in the jsonrpc4j module.

fixed

@wu-sheng
Copy link
Member

@apache/skywalking-committers Any of you has experience about this framework? I am concerned about who could review this.

@gy09535
Copy link
Contributor Author

gy09535 commented Apr 13, 2021

Read the plugin contribution document, we require the integration plugin test.
And as this is a personal project, I need to do some research whether we should accept this as an official plugin.

got it.

@gy09535 gy09535 changed the title feature: jsonrpc4j plugin WIP(feature): jsonrpc4j plugin Apr 14, 2021
@wu-sheng
Copy link
Member

And as this is a personal project, I need to do some research whether we should accept this as an official plugin.

I took a look at https://github.com/briandilley/jsonrpc4j, it seems alive for years, even it doesn't have a big contribution community but keep in healthy status.
It is a green light for this plugin.

@wu-sheng
Copy link
Member

Please fix your CI, and your task should be added into CI process.

@gy09535 gy09535 changed the title WIP(feature): jsonrpc4j plugin feature: jsonrpc4j plugin Apr 15, 2021
@wu-sheng wu-sheng added the feature New feature label Apr 15, 2021
@codecov
Copy link

codecov bot commented Apr 15, 2021 •
edited

Codecov Report

Merging #6743 (456e108) into master (eba7a5f) will increase coverage by 4.87%.
The diff coverage is 76.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6743      +/-   ##
============================================
+ Coverage     47.56%   52.43%   +4.87%     
- Complexity     3485     4061     +576     
============================================
  Files          1803     1810       +7     
  Lines         38591    38819     +228     
  Branches       4253     4257       +4     
============================================
+ Hits          18355    20356    +2001     
+ Misses        19225    17451    -1774     
- Partials       1011     1012       +1
Impacted Files Coverage Δ Complexity Δ
...jsonrpc4j/JsonRpcBasicServerInvokeInterceptor.java 57.14% <57.14%> (ø) 0.00 <0.00> (?)
...JsonRpcHttpClientPrepareConnectionInterceptor.java 61.53% <61.53%> (ø) 0.00 <0.00> (?)
...plugin/jsonrpc4j/JsonRpcHttpClientInterceptor.java 72.00% <72.00%> (ø) 0.00 <0.00> (?)
...ugin/jsonrpc4j/JsonServiceExporterInterceptor.java 90.47% <90.47%> (ø) 0.00 <0.00> (?)
.../apm/network/trace/component/ComponentsDefine.java 98.86% <100.00%> (+0.01%) 1.00 <0.00> (ø)
...walking/apm/plugin/jsonrpc4j/JsonRpcConstants.java 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...ywalking/apm/plugin/jsonrpc4j/JsonRpcPeerInfo.java 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...storage/plugin/jdbc/tidb/TiDBHistoryDeleteDAO.java 0.00% <0.00%> (-81.82%) 0.00% <0.00%> (-6.00%)
...org/apache/skywalking/apm/toolkit/meter/Gauge.java 25.00% <0.00%> (-75.00%) 0.00% <0.00%> (ø%)
.../storage/plugin/jdbc/tidb/TiDBStorageProvider.java 8.33% <0.00%> (-75.00%) 3.00% <0.00%> (-6.00%)
... and 184 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eba7a5f...456e108. Read the comment docs.

@gy09535
Copy link
Contributor Author

gy09535 commented Apr 16, 2021

JsonRpc4j plugin unit test and e2e test is pass, but some other CI is not pass.

wu-sheng
wu-sheng reviewed Apr 16, 2021
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Supported-list.md should be updated.
  2. CHANGES.md should be updated.

...j-1.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/jsonrpc4j/JsonRpcServeTests.java Outdated Show resolved Hide resolved
test/plugin/scenarios/jsonrpc4j-1.x-scenario/support-version.list Show resolved Hide resolved
...etwork/src/main/java/org/apache/skywalking/apm/network/trace/component/ComponentsDefine.java Show resolved Hide resolved
apm-sniffer/apm-sdk-plugin/jsonrpc4j-1.x-plugin/pom.xml Outdated Show resolved Hide resolved
...n/src/main/java/org/apache/skywalking/apm/plugin/jsonrpc4j/JsonRpcHttpClientInterceptor.java Show resolved Hide resolved
@wu-sheng
Copy link
Member

Please follow other review comments.

wu-sheng
wu-sheng approved these changes Apr 17, 2021
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good to me. Wait for CI tests. @kezhenxu94 Recheck?

@wu-sheng wu-sheng added this to the 8.6.0 milestone Apr 17, 2021
ascrutae
ascrutae approved these changes Apr 17, 2021
View reviewed changes
Copy link
Member

@ascrutae ascrutae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wu-sheng wu-sheng merged commit e898886 into apache:master Apr 17, 2021
@gy09535 gy09535 deleted the feature/jsonrpc4j-plugin branch April 18, 2021 11:27
@lujiajing1126 lujiajing1126 mentioned this pull request Oct 12, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

@ascrutae ascrutae ascrutae approved these changes

@zifeihan zifeihan Awaiting requested review from zifeihan

@zhaoyuguang zhaoyuguang Awaiting requested review from zhaoyuguang

@kezhenxu94 kezhenxu94 Awaiting requested review from kezhenxu94

Assignees
No one assigned
Labels
agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Milestone
8.6.0
Development

Successfully merging this pull request may close these issues.

feat: add json rpc plugin
4 participants
@gy09535 @wu-sheng @ascrutae @kezhenxu94