You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "xlq20080808 (via GitHub)" <gi...@apache.org> on 2023/03/03 06:18:35 UTC
[GitHub] [skywalking] xlq20080808 opened a new pull request, #10476: Feature DCS supports global configuration
xlq20080808 opened a new pull request, #10476:
URL: https://github.com/apache/skywalking/pull/10476
### DCS supports global configuration
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [x] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10461 .
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
The priority of each service configuration is higher than the global configuration, format like this
```
configurations:
// NEW added global config
key1 : value1
key2: value2
...
//service name
serviceA:
// Configurations of service A
// Key and Value are determined by the agent side.
// Check the agent setup doc for all available configurations.
key1: value1
key2: value2
...
serviceB:
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1476208961
Close for now, as this has not been updated for a week. If you want to continuous on this, please polish the codes, the logic is clear, we just need the codes better. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1459113137
@xlq20080808 Are you following this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#discussion_r1125459434
##########
oap-server/server-receiver-plugin/configuration-discovery-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/recevier/configuration/discovery/AgentConfigurationsReader.java:
##########
@@ -54,6 +54,9 @@ public AgentConfigurationsTable readAgentConfigurationsTable() {
if (configurationsData != null) {
configurationsData.forEach((k, v) -> {
Map map = (Map) v;
+ if (configurationsData.containsKey("default-config")) {
+ ((Map) configurationsData.get("default-config")).forEach(map::putIfAbsent);
+ }
Review Comment:
I am confused about this part. As `default-config` is a reserved key, and it shares the same structure as other services. Why do we have to change the init logic?
Also, we don't build `agentConfigurationsTable` properly(no UUID part).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#discussion_r1133522728
##########
oap-server/server-receiver-plugin/configuration-discovery-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/recevier/configuration/discovery/AgentConfigurations.java:
##########
@@ -37,9 +41,23 @@ public class AgentConfigurations {
*/
private volatile String uuid;
+ private volatile boolean needMerge = true;
+
public AgentConfigurations(final String service, final Map<String, String> configuration, final String uuid) {
this.service = service;
this.configuration = configuration;
this.uuid = uuid;
}
+
+ public void mergeAgentConfigurations(Map<String, String> config) {
Review Comment:
Actually, you don't need this. The only thing `ConfigurationDiscoveryServiceHandler#newAgentDynamicConfigCommand` required is `getConfiguration`. You could build an iterator by including both config.
You could do `agentConfigurations.buildConfiguraion(agentConfigurationsWatcher.getAgentConfigurations("default-config"))`.`foreach`.
By this, you don't need `needMerge`, also don't need to really merge two maps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] xlq20080808 commented on pull request #10476: Feature DCS supports global configuration
Posted by "xlq20080808 (via GitHub)" <gi...@apache.org>.
xlq20080808 commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1459401702
Thank you for your suggestion. I will modify it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] xlq20080808 commented on pull request #10476: Feature DCS supports global configuration
Posted by "xlq20080808 (via GitHub)" <gi...@apache.org>.
xlq20080808 commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1459150971
@wu-sheng yes i'm following
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] StruggleYang commented on pull request #10476: Feature DCS supports global configuration
Posted by "StruggleYang (via GitHub)" <gi...@apache.org>.
StruggleYang commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1562221869
Is this feature under further development? I found that I needed this configuration recently because I needed to globally exclude some endpoints from capture
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng closed pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng closed pull request #10476: Feature DCS supports global configuration
URL: https://github.com/apache/skywalking/pull/10476
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#discussion_r1125459567
##########
oap-server/server-receiver-plugin/configuration-discovery-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/recevier/configuration/discovery/AgentConfigurationsWatcher.java:
##########
@@ -73,7 +73,7 @@ public String value() {
public AgentConfigurations getAgentConfigurations(String service) {
AgentConfigurations agentConfigurations = agentConfigurationsTable.getAgentConfigurationsCache().get(service);
if (null == agentConfigurations) {
- return emptyAgentConfigurations;
+ return agentConfigurationsTable.getAgentConfigurationsCache().getOrDefault("default-config", emptyAgentConfigurations);
Review Comment:
This logic should work directly, isn't it? As from YAML perspective, `default-config` is a `service name` only.
##########
oap-server/server-receiver-plugin/configuration-discovery-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/recevier/configuration/discovery/AgentConfigurationsWatcher.java:
##########
@@ -73,7 +73,7 @@ public String value() {
public AgentConfigurations getAgentConfigurations(String service) {
AgentConfigurations agentConfigurations = agentConfigurationsTable.getAgentConfigurationsCache().get(service);
if (null == agentConfigurations) {
- return emptyAgentConfigurations;
+ return agentConfigurationsTable.getAgentConfigurationsCache().getOrDefault("default-config", emptyAgentConfigurations);
Review Comment:
Please correct me I missed anything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10476: Feature DCS supports global configuration
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1453053739
I also suggest using a hardcoded key to represent global configs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1459257985
OK, I think I got your point. I would suggest you enhance `AgentConfigurations` rather than pushing all configurations into the service-oriented configuration. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] xlq20080808 commented on a diff in pull request #10476: Feature DCS supports global configuration
Posted by "xlq20080808 (via GitHub)" <gi...@apache.org>.
xlq20080808 commented on code in PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#discussion_r1128918186
##########
oap-server/server-receiver-plugin/configuration-discovery-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/recevier/configuration/discovery/AgentConfigurationsReader.java:
##########
@@ -54,6 +54,9 @@ public AgentConfigurationsTable readAgentConfigurationsTable() {
if (configurationsData != null) {
configurationsData.forEach((k, v) -> {
Map map = (Map) v;
+ if (configurationsData.containsKey("default-config")) {
+ ((Map) configurationsData.get("default-config")).forEach(map::putIfAbsent);
+ }
Review Comment:
We need to add the missing configuration items of each service in default-config to the each service configuration, I think they should be similar to union relationship. And then we will traverse the map to generate UUID for each service
##########
oap-server/server-receiver-plugin/configuration-discovery-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/recevier/configuration/discovery/AgentConfigurationsWatcher.java:
##########
@@ -73,7 +73,7 @@ public String value() {
public AgentConfigurations getAgentConfigurations(String service) {
AgentConfigurations agentConfigurations = agentConfigurationsTable.getAgentConfigurationsCache().get(service);
if (null == agentConfigurations) {
- return emptyAgentConfigurations;
+ return agentConfigurationsTable.getAgentConfigurationsCache().getOrDefault("default-config", emptyAgentConfigurations);
Review Comment:
I think the priority is this. If there is a configuration for a single service, it is returned, and it also carries the configuration after the union with `default config`. If there is no separate configuration, the `default-config` is returned. If `default-config` is not set, the empty configuration is returned
##########
oap-server/server-receiver-plugin/configuration-discovery-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/recevier/configuration/discovery/AgentConfigurationsWatcher.java:
##########
@@ -73,7 +73,7 @@ public String value() {
public AgentConfigurations getAgentConfigurations(String service) {
AgentConfigurations agentConfigurations = agentConfigurationsTable.getAgentConfigurationsCache().get(service);
if (null == agentConfigurations) {
- return emptyAgentConfigurations;
+ return agentConfigurationsTable.getAgentConfigurationsCache().getOrDefault("default-config", emptyAgentConfigurations);
Review Comment:
Please correct my mistakes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] xlq20080808 commented on pull request #10476: Feature DCS supports global configuration
Posted by "xlq20080808 (via GitHub)" <gi...@apache.org>.
xlq20080808 commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1454611573
I agree, I have modified
format like this
configurations:
// NEW added global config
default-config:
key1 : value1
key2: value2
...
//service name
serviceA:
// Configurations of service A
// Key and Value are determined by the agent side.
// Check the agent setup doc for all available configurations.
key1: value1
key2: value2
...
serviceB:
...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1453047382
Do you need a (default-config) as a hard coded key? Rather than putting things in the root?
I feel the codes are more complex than I thought.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10476: Feature DCS supports global configuration
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1469045412
I think you still make the code boundaries not clear, which makes me hard to approve this change to upstream.
Let me try to re-word how I think the codes should be.
1. The default config should be held inside every other AgentConfigurations, rather than read every time.
2. You don't need the AgentConfigurations for the default config as the map is in other AgentConfigurations already.
3. UUID should be built inside the constructor as we need to combine the value of service-specific and default configs.
4. Build a temp iterator, rather than using streaming to merge those maps, which doesn't provide benefits but payload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] xlq20080808 commented on pull request #10476: Feature DCS supports global configuration
Posted by "xlq20080808 (via GitHub)" <gi...@apache.org>.
xlq20080808 commented on PR #10476:
URL: https://github.com/apache/skywalking/pull/10476#issuecomment-1465615031
Try to move the merging of config to ConfigurationDiscoveryServiceHandler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org