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

Agent service discorvery #5120

Closed
wants to merge 23 commits into from
Closed

Conversation

EvanLjp
Copy link
Member

@EvanLjp EvanLjp commented Jul 17, 2020

Please answer these questions before submitting pull request


New feature or improvement

  • Describe the details and related test reports.

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 17, 2020

I will add unit tests later, thanks for review first

@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #5120 into master will decrease coverage by 0.13%.
The diff coverage is 63.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5120      +/-   ##
============================================
- Coverage     53.08%   52.94%   -0.14%     
+ Complexity     3166     3163       -3     
============================================
  Files           820      822       +2     
  Lines         20508    20531      +23     
  Branches       1972     1976       +4     
============================================
- Hits          10887    10871      -16     
- Misses         8739     8776      +37     
- Partials        882      884       +2     
Impacted Files Coverage Δ Complexity Δ
...apache/skywalking/apm/agent/core/boot/Address.java 35.71% <35.71%> (ø) 2.00 <2.00> (?)
.../agent/core/discovery/DefaultDiscoveryService.java 76.92% <76.92%> (ø) 7.00 <7.00> (?)
...king/apm/agent/core/remote/GRPCChannelManager.java 55.40% <88.88%> (-1.01%) 13.00 <1.00> (+1.00) ⬇️
...er/core/server/auth/AuthenticationInterceptor.java 0.00% <0.00%> (-61.54%) 0.00% <0.00%> (-3.00%)
...er/sharing/server/ReceiverGRPCHandlerRegister.java 30.76% <0.00%> (-46.16%) 2.00% <0.00%> (-3.00%)
...ap/server/core/server/GRPCHandlerRegisterImpl.java 62.50% <0.00%> (-25.00%) 2.00% <0.00%> (-1.00%)
...r/cluster/plugin/standalone/StandaloneManager.java 77.77% <0.00%> (-22.23%) 3.00% <0.00%> (-1.00%)
...apm/agent/core/remote/ServiceManagementClient.java 60.37% <0.00%> (-13.21%) 9.00% <0.00%> (-1.00%)
...rg/apache/skywalking/apm/agent/core/os/OSUtil.java 62.71% <0.00%> (-5.09%) 10.00% <0.00%> (-2.00%)
...ing/oap/server/library/server/grpc/GRPCServer.java 55.93% <0.00%> (-5.09%) 6.00% <0.00%> (-1.00%)
... 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 118e334...1c83209. Read the comment docs.

@kezhenxu94 kezhenxu94 linked an issue Jul 17, 2020 that may be closed by this pull request
1 task
@kezhenxu94 kezhenxu94 requested a review from wu-sheng July 17, 2020 12:02
@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 17, 2020

fixing unit test,please wait for a moment

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.

I think I mentioned before, neither service discovery tech should not be bounded in the agemt core level. If you want to contribute this feature, you need to put it as an optional plugin only. Take a look on Kafka transport plugin and zipkin branch.

@wu-sheng
Copy link
Member

Don't change the backend serive url config, it will break many deployed system. Please don't do that.

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 17, 2020

I think I mentioned before, neither service discovery tech should not be bounded in the agemt core level. If you want to contribute this feature, you need to put it as an optional plugin only. Take a look on Kafka transport plugin and zipkin branch.
@wu-sheng
it is designed as a plugin mode:
image

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 17, 2020

so i cannot extension with service mode , i would be add a new extension interface ?

@wu-sheng
Copy link
Member

The concern is, the default config changed. And new backend service path added. This would be very confused. How to write plugin doc would be an issue.

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 17, 2020

The concern is, the default config changed. And new backend service path added. This would be very confused. How to write plugin doc would be an issue.

how about this mode:
origin backend url is as default.the extension discovery is in Service Discovery
image

@wu-sheng wu-sheng added agent Language agent related. feature New feature TBD To be decided later, need more discussion or input. labels Jul 17, 2020
@wu-sheng
Copy link
Member

By thinking the PR and Kafka transport PR, I prefer you work on a new PR.
Introduce a new SPI service, called ConfigInitService. All plugins and core could provide their services. Then you could separate the current Config in the core and different plugins.
Notice, all config keys should be kept as much as possible.

What do you think? @dmsolr @kezhenxu94 @arugal @EvanLjp

Let's make that decision first, then back to this feature.

@wu-sheng
Copy link
Member

@EvanLjp Yes, this one and #4847 show, the config of different plugins have been keeping added into the core Config. I want to remove this first. That is the reason I talked about ConfigInitService(not a BootService).

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 17, 2020

i open a new issue first

@wu-sheng
Copy link
Member

Once we have this feature, this plugin will be more elegant, because there will be no code changed in the agent core, purely plugin. Same as the Kafka transport plugin.

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 17, 2020

I wonder if the plug-in configuration needs to be refactored,for example mysql

@wu-sheng
Copy link
Member

I wonder if the plug-in configuration needs to be refactored,for example mysql

At the code level, yes. But you still could keep the config name unchanged. You just create more Config classes in different plugins.

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 20, 2020

no porblem , i would like be to open a new issue first

@wu-sheng
Copy link
Member

I think you could improve this PR now. I have set up the new Config initialization core.

@EvanLjp
Copy link
Member Author

EvanLjp commented Jul 21, 2020

ok

# Conflicts:
#	apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java
#	apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java
#	apm-sniffer/apm-agent-core/src/main/resources/META-INF/services/org.apache.skywalking.apm.agent.core.boot.BootService
#	apm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/boot/ServiceManagerTest.java
@wu-sheng
Copy link
Member

wu-sheng commented Aug 1, 2020

Do we still plan to do this? I think we discussed, cliemt load balance is not a very good way. It is hard to be balaned.

@EvanLjp
Copy link
Member Author

EvanLjp commented Aug 1, 2020

i read the oap code and agent code. oap transform data from one oap server to another oap server is get client form a grpc client pool. so oap server can load balance.
why not we still maintain the mode in agent.
we create a grpc clients manager,when transform data ,we random get a client

@EvanLjp
Copy link
Member Author

EvanLjp commented Aug 1, 2020

Do we still plan to do this? I think we discussed, cliemt load balance is not a very good way. It is hard to be balaned.

i don't think keep alive one by one is a good way.Don't you think this is not a skywalking's current problem?Why don't we try to optimize?

@wu-sheng
Copy link
Member

Rechecking, whether this feature should be or is planned to continue? It has been no update for a month.

@EvanLjp
Copy link
Member Author

EvanLjp commented Aug 31, 2020

may be close is a good choose, thx

@EvanLjp EvanLjp closed this Aug 31, 2020
@wu-sheng wu-sheng added this to the 8.2.0 milestone Aug 31, 2020
@wu-sheng wu-sheng added wontfix This will not be worked on and removed TBD To be decided later, need more discussion or input. labels Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. feature New feature wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build connection between agent client and oap server with service discover
2 participants