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.

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

Remove the local/exit span register mechanism in Java agent scenario #4059

Merged
merged 21 commits into from
Dec 15, 2019

Conversation

wu-sheng
Copy link
Member

Resolves #4056

Key, reduce the load of register and amount of operation id. Also, reduce the agent memory cost.
At the same time, the local span and exit span plugins will not concern about parameter in the op name causing too many registers issue.

@kezhenxu94 Could you review and run a test locally? Especially for the plugin you are working on, Runnable case I mean. If inside runnable, an RPC is called, the endpoint dependency should show that endpoint still depends on the parent endpoint(RPC in the parent thread of the runnable). If no parent thread exist, as this thread is a local task(timer), then the endpoint should depend on the user. Could you verify these two cases? And confirm no local span and exit span register happen, check through

  1. No local and exit operation name in the endpoint inventory
  2. No warning log, like this Unexpected endpoint register, endpoint isn't detected from server side. ,happens.

@wu-sheng wu-sheng added core feature Core and important feature. Sometimes, break backwards compatibility. agent Language agent related. backend OAP backend related. labels Dec 14, 2019
@wu-sheng wu-sheng added this to the 6.6.0 milestone Dec 14, 2019
@wu-sheng
Copy link
Member Author

We include exit span in the plugin test cases, I have to remove them one by one, sadly....

@codecov-io
Copy link

codecov-io commented Dec 14, 2019

Codecov Report

Merging #4059 into master will decrease coverage by 0.03%.
The diff coverage is 23.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4059      +/-   ##
==========================================
- Coverage   26.85%   26.82%   -0.04%     
==========================================
  Files        1134     1134              
  Lines       24980    24986       +6     
  Branches     3601     3607       +6     
==========================================
- Hits         6709     6703       -6     
- Misses      17674    17686      +12     
  Partials      597      597
Impacted Files Coverage Δ
...r/parser/standardization/ReferenceIdExchanger.java 0% <ø> (ø) ⬆️
.../apache/skywalking/apm/agent/core/conf/Config.java 72.34% <ø> (-0.58%) ⬇️
...r/receiver/trace/provider/parser/SegmentParse.java 0% <0%> (ø) ⬆️
...ser/listener/endpoint/MultiScopesSpanListener.java 0% <0%> (ø) ⬆️
...receiver/trace/provider/parser/SegmentParseV2.java 0% <0%> (ø) ⬆️
...ing/resttemplate/async/RestExecuteInterceptor.java 0% <0%> (ø) ⬆️
...ring/resttemplate/sync/RestExecuteInterceptor.java 0% <0%> (ø) ⬆️
...provider/parser/standardization/SpanExchanger.java 0% <0%> (ø)
...r/parser/listener/segment/SegmentSpanListener.java 0% <0%> (ø) ⬆️
...ovider/handler/v6/grpc/RegisterServiceHandler.java 0% <0%> (ø) ⬆️
... and 5 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 23311ca...dc7d6c8. Read the comment docs.

@wu-sheng
Copy link
Member Author

@dmsolr @wayilau @arugal I already updated the plugin test document and existing plugin test cases. Please review this PR too. If this PR could pass @kezhenxu94 and @ascrutae 's local tests, I would like to merge this ASAP, due to it affects all potential and existing plugin test PRs.

@wu-sheng
Copy link
Member Author

FYI @cyejing

.setFrom(endpoint.getFrom()));
}
} else {
logger.warn("Unexpected endpoint register, endpoint isn't detected from server side. {}", request);
Copy link
Member Author

Choose a reason for hiding this comment

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

To all reviewers, this needs to be added into changelog and FAQ, as people may face a lot of these logs when agents have not upgraded. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just submit a commit to do this. please recheck.

@wu-sheng
Copy link
Member Author

@kezhenxu94 @ascrutae All tests passed, but I think you should run a local test to re-verify.

@arugal
Copy link
Member

arugal commented Dec 14, 2019

@wu-sheng Hi, go2sky needs to be modified?

@wu-sheng
Copy link
Member Author

Hi, go2sky needs to be modified?

You could recheck. In my mind, all other agents and SDK are not using register at the SDK side.

@arugal
Copy link
Member

arugal commented Dec 14, 2019

Hi, go2sky needs to be modified?

You could recheck. In my mind, all other agents and SDK are not using register at the SDK side.

Checked, go2sky does not register endpoint.

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.

@kezhenxu94 Could you review and run a test locally? Especially for the plugin you are working on, Runnable case I mean. If inside runnable, an RPC is called, the endpoint dependency should show that endpoint still depends on the parent endpoint(RPC in the parent thread of the runnable).

Checked, LGTM

@wu-sheng wu-sheng merged commit 4e50132 into master Dec 15, 2019
@wu-sheng wu-sheng deleted the reduce-register-load branch December 15, 2019 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. backend OAP backend related. core feature Core and important feature. Sometimes, break backwards compatibility.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove local span and exit span OP name register permanently
5 participants