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

Add the plugin for mssql-jtds 1.x plugin. #5842

Merged
merged 6 commits into from Nov 16, 2020
Merged

Add the plugin for mssql-jtds 1.x plugin. #5842

merged 6 commits into from Nov 16, 2020

Conversation

zifeihan
Copy link
Member

@zifeihan zifeihan commented Nov 14, 2020 •
edited by wu-sheng

Add an agent plugin to support mssql-jtds 1.x

Fix #2340, add an agent plugin to support mssql-jtds 1.x, like
image
image
image

@codecov
Copy link

codecov bot commented Nov 14, 2020 •
edited

Codecov Report

Merging #5842 (1bbd216) into master (ef1363d) will decrease coverage by 11.67%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #5842       +/-   ##
=============================================
- Coverage     52.59%   40.92%   -11.68%     
+ Complexity     3465     2694      -771     
=============================================
  Files           897      937       +40     
  Lines         22475    23221      +746     
  Branches       2150     2232       +82     
=============================================
- Hits          11821     9503     -2318     
- Misses         9704    13019     +3315     
+ Partials        950      699      -251
Impacted Files Coverage Δ Complexity Δ
.../apm/network/trace/component/ComponentsDefine.java 0.00% <0.00%> (-98.81%) 0.00 <0.00> (-1.00)
...ywalking/apm/agent/core/logging/core/LogLevel.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...ywalking/apm/agent/core/plugin/EnhanceContext.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-5.00%)
...ywalking/apm/agent/core/profile/ProfileStatus.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...walking/apm/agent/core/context/SW8CarrierItem.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...walking/apm/agent/core/jvm/cpu/SunCpuAccessor.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...walking/apm/agent/core/logging/core/LogOutput.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...walking/apm/agent/core/plugin/match/NameMatch.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...alking/apm/agent/core/context/CarrierItemHead.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...alking/apm/agent/core/jvm/gc/ParallelGCModule.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
... and 207 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 ef1363d...1bbd216. Read the comment docs.

@wu-sheng wu-sheng requested review from kezhenxu94 and dmsolr and removed request for kezhenxu94 November 14, 2020 14:24
@wu-sheng 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. labels Nov 14, 2020
kezhenxu94
kezhenxu94 requested changes Nov 15, 2020
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment •
edited

Choose a reason for hiding this comment

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

UPDATED

Resolved since it's provided scope

@zifeihan https://github.com/milesibastos/jTDS checked this lib, it's under license LGPL 2.1, which is not allowed to be included in this repository, maybe we need to move this plugin to the repo https://github.com/SkyAPM/java-plugin-extensions

apm-sniffer/apm-sdk-plugin/mssql-jtds-1.x-plugin/pom.xml Outdated Show resolved Hide resolved
@wu-sheng
Copy link
Member

https://github.com/milesibastos/jTDS checked this lib, it's under license LGPL 2.1, which is not allowed to be included in this repository

@kezhenxu94 LGPL compiling is allowed, the key is provided, which should be fixed. The plugins of LGPL in that repo of LGPL is because it is not accessible from Maven Central. We can't download it.

@wu-sheng
Copy link
Member

The key to GPL/LGPL dependency restriction is, we can't ask the user has to use this lib. But in the plugin system, we don't depend on the lib, the user did before use, we are just adopting, which is fine.

@wu-sheng
Copy link
Member

Also, at the same time, I assume the logo is not allowed(rocketbot-ui), it is not just GPL/LGPL, it is totally commercial.

@kezhenxu94
Copy link
Member

https://github.com/milesibastos/jTDS checked this lib, it's under license LGPL 2.1, which is not allowed to be included in this repository

@kezhenxu94 LGPL compiling is allowed, the key is provided, which should be fixed. The plugins of LGPL in that repo of LGPL is because it is not accessible from Maven Central. We can't download it.

Got it. Thanks for clarifying. Will continue to review then

kezhenxu94
kezhenxu94 reviewed Nov 15, 2020
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Some nits inline

.github/workflows/plugins-test.0.yaml Outdated Show resolved Hide resolved
...va/org/apache/skywalking/apm/plugin/mssql/jtds/v1/define/ConnectionJDBC2Instrumentation.java Outdated Show resolved Hide resolved
kezhenxu94
kezhenxu94 previously approved these changes Nov 15, 2020
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thanks. Codes and test results look good to me

...org/apache/skywalking/apm/plugin/mssql/jtds/v1/define/AbstractConnectionInstrumentation.java Outdated Show resolved Hide resolved
@kezhenxu94
Update apm-sniffer/apm-sdk-plugin/mssql-jtds-1.x-plugin/src/main/java…
9883ac8 
…/org/apache/skywalking/apm/plugin/mssql/jtds/v1/define/AbstractConnectionInstrumentation.java

Co-authored-by: Zhenxu Ke <kezhenxu94@163.com>
@wu-sheng wu-sheng added this to the 8.3.0 milestone Nov 15, 2020
kezhenxu94
kezhenxu94 approved these changes Nov 15, 2020
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thanks, good for me,

@wu-sheng wu-sheng merged commit 7220643 into apache:master Nov 16, 2020
@zifeihan zifeihan deleted the mssql branch November 16, 2020 03:03
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

@kezhenxu94 kezhenxu94 kezhenxu94 approved these changes

@dmsolr dmsolr Awaiting requested review from dmsolr

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.3.0
Development

Successfully merging this pull request may close these issues.

When Skywallking supports MSSQL ?
3 participants
@zifeihan @wu-sheng @kezhenxu94