-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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.
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
feature: support apm with skywalking #3652
Conversation
@wu-sheng , @zhaoyuguang What do you think of using ASF or Seata license header in code files? The Seata community supports the both and fully respects intellectual property rights. |
From my understanding, all codes contributed here should be licensed to the Seata community. SkyWalking just provides the tech stack, dependencies. If you have a LICENSE file for the binary distribution, please add Apache SkyWalking in it. |
Do you mean that it needs to add Apache Skywalking in https://github.com/seata/seata/blob/1ec5e031de2de113859b74789f2b72ea91c1cf75/distribution/LICENSE-BIN ? |
Yes, take this as an example of the official Apache Software Foundation's way. https://github.com/apache/skywalking/blob/master/dist-material/release-docs/LICENSE#L204 |
cc @zhaoyuguang |
Codecov Report
@@ Coverage Diff @@
## develop #3652 +/- ##
=============================================
- Coverage 51.28% 51.25% -0.03%
+ Complexity 3573 3570 -3
=============================================
Files 645 645
Lines 21814 21814
Branches 2740 2736 -4
=============================================
- Hits 11187 11181 -6
- Misses 9487 9496 +9
+ Partials 1140 1137 -3
|
@wu-sheng From the Seata project package style, I want to change the plugin package to |
Yes, of course, it is better to use seata package name, as they host codes. |
BTW, please remember to update Seata website/doc page, about how to use this. |
Yes, the PR only focused on plugin monitoring code, Next is the construction of official website documents |
Is this going to merge now? Is this ready? @slievrly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhaoyuguang Please modify the LICENSE-BIN under the distribution
folder at the meantime
It's already in the pipeline process of code review and function test. Assigned to @caohdgege @selfishlover . |
Hi, I want to give a reminder, DolphinScheduler has a SkyWalking agent installation out of box. https://dolphinscheduler.apache.org/en-us/docs/latest/user_doc/skywalking-agent-deployment.html A user just needs a system env setup, or several new lines in Right now, this PR seems doesn't have related things. Then, the user will need several manual steps to make all things ready to go. At the same time, only because of using that way, you should update the LICENSE. If you only host plugin source codes and binary, both of them actually belong to the Seata project, rather than SkyWalking. SkyWalking becomes a compiling level dependency. @zhaoyuguang @slievrly I think from the original discussion and issue description, we should have skywalking agent with all necessary plugins(not all skywalking's plugins) packaged into Seata's distribution. Could you confirm this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If you want, could add one in the component library YAML file. |
It is my fault. I have seen the above issue comment (#3652 (comment). |
This was discussed at the Seata monthly meeting. Seata-server is an independently deployed application. As a user-friendly way, skywalking plugin should be included in the distribution package of Seata-server. When executing I think this function can be optimized in the next pr. |
...-skywalking-plugin/src/main/java/io/seata/apm/skywalking/plugin/common/SWSeataConstants.java
Show resolved
Hide resolved
…apm-sw-0 # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
I think |
In addition, too many SEATA/TC/HeartbeatMessage, it can be ignored, it just keep channel alive and connected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In my mind, a health check should be a meter plugin, rather than tracing. |
thanks @wu-sheng @zhaoyuguang @kezhenxu94 @skywalking A great thing has been accomplished. |
Look forward to your blog sharing how to use this to diagnose Seata. |
Ⅰ. Describe what this PR did
support apm with skywalking
Ⅱ. Does this pull request fix one issue?
#714
#884
apache/skywalking#6579
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews