[GitHub] [skywalking] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script

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

yswdqz commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967673964


##########
oap-server/server-starter/src/main/resources/lal/default.yaml:
##########
@@ -16,6 +16,7 @@
 # The default LAL script to save all logs, behaving like the versions before 8.5.0.
 rules:
   - name: default
+    layer: UNDEFINED

Review Comment:
   I don't know how to set the layer of default.yaml.
   It should be GENERAL, but as a default rule, maybe UNDEFINED is more suitable?



##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/LogAnalyzerModuleConfig.java:
##########
@@ -51,7 +51,7 @@ public class LogAnalyzerModuleConfig extends ModuleConfig {
     private List<Rule> meterConfigs;
 
     public List<String> lalFiles() {
-        return Splitter.on(",").omitEmptyStrings().splitToList(Strings.nullToEmpty(getLalFiles()));
+        return Splitter.on(",").omitEmptyStrings().trimResults().splitToList(Strings.nullToEmpty(getLalFiles()));

Review Comment:
   I change this method because I found that it can not split strings such as "a, b" before. It will split as "a" and "<space>b".



##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,55 +19,65 @@
 package org.apache.skywalking.oap.log.analyzer.provider.log.listener;
 
 import com.google.protobuf.Message;
-import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 import java.util.stream.Collectors;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.collections4.map.HashedMap;
 import org.apache.skywalking.apm.network.logging.v3.LogData;
 import org.apache.skywalking.oap.log.analyzer.dsl.Binding;
 import org.apache.skywalking.oap.log.analyzer.dsl.DSL;
 import org.apache.skywalking.oap.log.analyzer.provider.LALConfig;
 import org.apache.skywalking.oap.log.analyzer.provider.LALConfigs;
 import org.apache.skywalking.oap.log.analyzer.provider.LogAnalyzerModuleConfig;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
 import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
 
 @Slf4j
 @RequiredArgsConstructor
 public class LogFilterListener implements LogAnalysisListener {
-    private final List<DSL> dsls;
+    private final Map<String, DSL> dsls;

Review Comment:
   I wan't to talk about the map should be
   ```
   // such as the string "MYSQL;"
   map<String, DSL>
   ```
   or 
   ```
   map<Layer, DSL>
   ```



##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,55 +19,65 @@
 package org.apache.skywalking.oap.log.analyzer.provider.log.listener;
 
 import com.google.protobuf.Message;
-import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 import java.util.stream.Collectors;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.collections4.map.HashedMap;
 import org.apache.skywalking.apm.network.logging.v3.LogData;
 import org.apache.skywalking.oap.log.analyzer.dsl.Binding;
 import org.apache.skywalking.oap.log.analyzer.dsl.DSL;
 import org.apache.skywalking.oap.log.analyzer.provider.LALConfig;
 import org.apache.skywalking.oap.log.analyzer.provider.LALConfigs;
 import org.apache.skywalking.oap.log.analyzer.provider.LogAnalyzerModuleConfig;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
 import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
 
 @Slf4j
 @RequiredArgsConstructor
 public class LogFilterListener implements LogAnalysisListener {
-    private final List<DSL> dsls;
+    private final Map<String, DSL> dsls;
+    private LogData.Builder logData;
 
     @Override
     public void build() {
-        dsls.forEach(dsl -> {
-            try {
-                dsl.evaluate();
-            } catch (final Exception e) {
-                log.warn("Failed to evaluate dsl: {}", dsl, e);
+        DSL dsl = dsls.get(logData.getLayer());
+        try {
+            if (dsl == null) {
+                if (StringUtil.isEmpty(logData.getLayer())) {
+                    log.debug("The layer is empty, will use default rules");
+                }
+                dsl = dsls.get(Layer.UNDEFINED.name());
             }
-        });
+            dsl.evaluate();
+        } catch (final Exception e) {
+            log.warn("Failed to evaluate dsl: {}", dsl, e);
+        }
     }
 
     @Override
     public LogAnalysisListener parse(final LogData.Builder logData,
                                      final Message extraLog) {
-        dsls.forEach(dsl -> dsl.bind(new Binding().log(logData.build())
+        dsls.forEach((layer, dsl) -> dsl.bind(new Binding().log(logData.build())
                                                   .extraLog(extraLog)));
+        this.logData = logData;
         return this;
     }
 
     public static class Factory implements LogAnalysisListenerFactory {
-        private final List<DSL> dsls;
+        private final Map<String, DSL> dsls;

Review Comment:
   I have thought about it. But I think if I add a param to Factory#create method will affect other two listeners(RecordAnalysisListener and TrafficAnalysisListener).
   Is that OK? They seems don't need layer param.



-- 
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