[GitHub] ScienJus commented on a change in pull request #1887: Support for overriding config with agent options

[GitBox <gi...@apache.org>]

ScienJus commented on a change in pull request #1887: Support for overriding config with agent options
URL: https://github.com/apache/incubator-skywalking/pull/1887#discussion_r231389782
 
 

 ##########
 File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java
 ##########
 @@ -84,6 +95,20 @@ public static void initialize() throws ConfigNotFoundException, AgentPackageNotF
         IS_INIT_COMPLETED = true;
     }
 
+    private static void overrideConfigByAgentOptions(String agentOptions) throws IllegalAccessException {
+        Properties properties = new Properties();
+        for (String entry : agentOptions.split(",")) {
+            String[] kvPair = entry.split("=");
+            // The separator for multiple values is ',' in the config file.
+            // But in agent options, it conflicts with the separator of multiple options, so we use ';'.
+            // Maybe we should change the separator of multiple options to ';'?
+            properties.put(kvPair[0], kvPair[1].replace(";", ","));
 
 Review comment:
   Hi @wu-sheng, it's just about the new agent options config, My first design uses ',' as separator, like:
   
   ```
   agent.application_code=testApp,collector.backend_service=127.0.0.1:8090,logging.level=info
   ``` 
   
   Then I saw that `agent.ignore_suffix` uses `,` split multiple suffixes, It will conflict with my logic. like:
   
   
   ```
   "agent.application_code=testApp,agent.ignore_suffix=.jpg,.png,.html".split(",")
   
   // [2], [3] are not key-val pairs
   ["agent.application_code=testApp","agent.ignore_suffix=.jpg", ".png", ".html"]
   ``` 
   
   So we have two solutions:
   
   1. change the separator of multiple options from ',' to ';' like:
   
   ```
   agent.application_code=testApp;collector.backend_service=127.0.0.1:8090;logging.level=info
   ```
   
   2. use ';' to split multiple values in single option, then replace to ',' in `overrideConfigByAgentOptions` (the current solution), the options will be like:
   
   ```
   agent.application_code=testApp;collector.backend_service=127.0.0.1:8090;logging.level=info,agent.ignore_suffix=.jpg;.jpeg;.js;.css;.png;.bmp;.gif;.ico;.mp3;.mp4;.html;.svg
   ```
   
   I think the second solution is better.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services