You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/09/10 16:16:05 UTC
[GitHub] [skywalking] yswdqz opened a new pull request, #9593: Add layer property to LAL script
yswdqz opened a new pull request, #9593:
URL: https://github.com/apache/skywalking/pull/9593
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967719096
##########
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 checked how this Factory is used, I feel you don't need this Map in the LogFilterListener. You could use `layer` as a parameter in `#create` method to build the suitable listener directly.
--
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] yswdqz commented on pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#issuecomment-1242879977
The deadlink check always fail and seems not cause by me.
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968222852
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
> @kezhenxu94 The reason is `LogFilterListener`, it is required to have a `layer` as parameter to create `listener`, which we changed in this PR.
>
> But once we add them, we have to change all others due to this hierarchy tree.
>
> <img alt="image" width="675" src="https://user-images.githubusercontent.com/5441976/189624683-9b7d0368-86cb-4d49-9c32-4788e114f1d7.png">
>
> The only reason I asked for this, is to make `create(null)` disappear.
That’s my point. How to separate the 3 factories? Add a new sink factory and the add the sink factory here?
https://github.com/apache/skywalking/blob/5ba874a49946571506b6cb3c1abbf0d4ce1863bf/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/ILogAnalysisListenerManager.java#L23
```yaml
void addSinkListenerFactory(LogSinkListenerFactory factory);
```
--
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] yswdqz commented on pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#issuecomment-1242928499
> Please check why log e2e are all failure.
I get 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] wu-sheng commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967816282
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,77 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
- }
- });
+ try {
+ 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())
- .extraLog(extraLog)));
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ if (dsls.put(layer, DSL.of(moduleManager, config, c.getDsl())) != null) {
+ log.warn("layer {} has already set a rule {}, the old one will be ignored.", layer, c.getName());
+ }
+ } catch (UnexpectedException e) {
+ log.warn("layer {} not found, will ignore this rule.", c.getName(), e);
Review Comment:
Layer should break the booting process too.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/LogAnalyzer.java:
##########
@@ -46,7 +50,20 @@ public void doAnalysis(LogData.Builder builder, Message extraLog) {
log.debug("The log is ignored because the Service name is empty");
return;
}
- createListeners();
+ Layer layer;
+ if ("".equals(builder.getLayer())) {
+ layer = Layer.GENERAL;
+ }
+ else {
+ try {
+ layer = Layer.nameOf(builder.getLayer());
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will skip this process.");
Review Comment:
```suggestion
log.warn(The Layer {} is not found, abandon the log.", builder.getLayer());
```
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,77 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
- }
- });
+ try {
+ 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())
- .extraLog(extraLog)));
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ if (dsls.put(layer, DSL.of(moduleManager, config, c.getDsl())) != null) {
+ log.warn("layer {} has already set a rule {}, the old one will be ignored.", layer, c.getName());
+ }
Review Comment:
Layer conflicts should cause booting failure. Please throw the exception with clear information about which rules(files) are using the same layer.
--
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 merged pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #9593:
URL: https://github.com/apache/skywalking/pull/9593
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967797050
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
I am wondering, why we use `null` here?
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967801764
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/LogAnalyzer.java:
##########
@@ -64,8 +79,8 @@ private void notifyListenerToBuild() {
listeners.forEach(LogAnalysisListener::build);
}
- private void createListeners() {
+ private void createListeners(Layer layer) {
factoryManager.getLogAnalysisListenerFactories()
- .forEach(factory -> listeners.add(factory.create()));
+ .forEach(factory -> listeners.add(factory.create(layer)));
Review Comment:
Maybe change to
```java
factoryManager.getLogAnalysisListenerFactories()
.stream()
.map(factory -> factory.create(layer))
.filter(Objects::nonNull)
.forEach(listeners::add);
```
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968212633
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
@kezhenxu94 The reason is `LogFilterListener`, it is required to have a `layer` as parameter to create `listener`, which we changed in this PR.
But once we add them, we have to change all others due to this hierarchy tree.
<img width="675" alt="image" src="https://user-images.githubusercontent.com/5441976/189624683-9b7d0368-86cb-4d49-9c32-4788e114f1d7.png">
The only reason I asked for this, is to make `create(null)` disappear.
--
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] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967819531
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,77 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
- }
- });
+ try {
+ 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())
- .extraLog(extraLog)));
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ if (dsls.put(layer, DSL.of(moduleManager, config, c.getDsl())) != null) {
+ log.warn("layer {} has already set a rule {}, the old one will be ignored.", layer, c.getName());
+ }
Review Comment:
DSL has not stored the name of rule. So it is difficult to log which rules(files) are using the same layer.
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#issuecomment-1243095080
I think in today's e2e log cases don't report layer as it was not required. It is better if we could add default=general layer to the log receiver before it is forwarded into LAL engine to keep forward compatibility.
--
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] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967766878
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
+ }
}
}
@Override
- public LogAnalysisListener create() {
- return new LogFilterListener(dsls);
+ public LogAnalysisListener create(Layer layer) {
+ if (layer == null) {
+ return null;
Review Comment:
I have checked it. No other invoker use the return value.
--
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] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967793724
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
+ }
}
}
@Override
- public LogAnalysisListener create() {
- return new LogFilterListener(dsls);
+ public LogAnalysisListener create(Layer layer) {
+ if (layer == null) {
+ return null;
Review Comment:
@kezhenxu94 I have tested a null value situation. It will not throw a Exception.
An ArrayList can store more than one null.
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967797886
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
@kezhenxu94 Should we separate `LogFilterListener.Factory` and `TrafficAnalysisListener.Factory`/`RecordAnalysisListener.Factory`?
`TrafficAnalysisListener.Factory`/`RecordAnalysisListener.Factory` are more likely `LogSinkListener`.
--
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] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967797208
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
These two listeners needn't 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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967779749
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,86 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ Layer layer = null;
+ try {
+ layer = Layer.nameOf(c.getLayer());
+ if (dsls.containsKey(layer)) {
+ log.warn("layer {} has set a rule, old one will be ignore.", c.getName());
+ }
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer {} not found, will ignore this rule.", c.getName());
+ }
Review Comment:
```suggestion
try {
Layer layer = Layer.nameOf(c.getLayer());
if (dsls.put(layer, DSL.of(moduleManager, config, c.getDsl())) != null) {
log.warn("layer {} has already set a rule {}, the old one will be ignored.", layer, c.getName());
}
} catch (UnexpectedException e) {
log.warn("layer {} not found, will ignore this rule.", c.getName(), e);
}
```
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,86 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
Review Comment:
`dsl` is non null.
```suggestion
```
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,86 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
Review Comment:
`dsl` is already non null.
```suggestion
```
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968227946
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
TrafficAnalysisListener#Factory and RecordAnalysisListener#Factory implement from a new interface, that is all.
--
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] yswdqz commented on pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#issuecomment-1242929093
Maybe because of the layer check I have set in this PR.
```
try {
layer = Layer.nameOf(builder.getLayer());
} catch (UnexpectedException e) {
log.warn("layer not found, will skip this process.");
return;
}
```
I think that I can set `GENERAL`as default layer.
--
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] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967802703
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
+ }
}
}
@Override
- public LogAnalysisListener create() {
- return new LogFilterListener(dsls);
+ public LogAnalysisListener create(Layer layer) {
+ if (layer == null) {
+ return null;
Review Comment:
I understand , it's my fault. I will change it soon.
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968217289
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
> Currently the reason why there is a null in the create method is that the 3 factories share the same interface and the interface is used in the core module of log analysis, if we could separate the sink factory we could just remove the null parameter from the create method.
I feel you are asking the same thing. 3 implementations for one interface. Add one for <2> and <3>, and add one for <1>? I just don't think they are doing the same things, as one for core, the other two for sink. And `null` as a `create` method's parameter seems not right.
I didn't insist on anything relative to which interface and the names. If you want any preference, feel free to propose.
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968263427
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
> > @kezhenxu94 The reason is `LogFilterListener`, it is required to have a `layer` as parameter to create `listener`, which we changed in this PR.
>
> >
>
> > But once we add them, we have to change all others due to this hierarchy tree.
>
> >
>
> > <img alt="image" width="675" src="https://user-images.githubusercontent.com/5441976/189624683-9b7d0368-86cb-4d49-9c32-4788e114f1d7.png">
>
> >
>
> > The only reason I asked for this, is to make `create(null)` disappear.
>
>
>
> That’s my point. How to separate the 3 factories? Add a new sink factory and then add the sink factory here?
>
>
>
> https://github.com/apache/skywalking/blob/5ba874a49946571506b6cb3c1abbf0d4ce1863bf/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/ILogAnalysisListenerManager.java#L23
>
>
>
> ```yaml
>
> void addSinkListenerFactory(LogSinkListenerFactory factory);
>
> ```
>
>
>
>
I am not sure whether we need. I remember those two factories are hard coded created.
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967757877
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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;
Review Comment:
Prefer `java.util.HashMap`
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
Review Comment:
`dsl` should be non null, otherwise you should not create this listener.
```suggestion
@lombok.NonNull
private final DSL dsl;
```
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
+ }
}
}
@Override
- public LogAnalysisListener create() {
- return new LogFilterListener(dsls);
+ public LogAnalysisListener create(Layer layer) {
+ if (layer == null) {
+ return null;
Review Comment:
Check the places where this method `create` is invoked, if you return null here, `NullPointerException` might be thrown, you have to check the nullability in the callers.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
Review Comment:
Now there is a limitation that only one LAL script can be configured for a layer, you might need to check here and throw exception if multiple DSLs are configured for a same layer
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
Review Comment:
```suggestion
log.warn("layer {} not found, will ignore this rule.", c.getName());
```
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
+ }
}
}
@Override
- public LogAnalysisListener create() {
- return new LogFilterListener(dsls);
+ public LogAnalysisListener create(Layer layer) {
+ if (layer == null) {
+ return null;
+ }
+ return new LogFilterListener(dsls.get(layer));
Review Comment:
```suggestion
final DSL dsl = dsls.get(layer);
if (dsl == null) {
return null;
}
return new LogFilterListener(dsl);
```
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967819706
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,77 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
- }
- });
+ try {
+ 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())
- .extraLog(extraLog)));
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ if (dsls.put(layer, DSL.of(moduleManager, config, c.getDsl())) != null) {
+ log.warn("layer {} has already set a rule {}, the old one will be ignored.", layer, c.getName());
+ }
Review Comment:
> DSL has not stored the name of rule. So it is difficult to log which rules(files) are using the same layer.
Simply log the layer should be enough IMO
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967802374
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
> @kezhenxu94 Should we separate `LogFilterListener.Factory` and `TrafficAnalysisListener.Factory`/`RecordAnalysisListener.Factory`?
>
> `TrafficAnalysisListener.Factory`/`RecordAnalysisListener.Factory` are more likely `LogSinkListener`.
Hi, I'm not sure, these factories share the same interface and they are used basically in the core log analyzer, so splitting these might bring huge changes.
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967822688
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,73 @@
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 java.util.HashMap;
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ @lombok.NonNull
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
- }
- });
+ try {
+ 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())
- .extraLog(extraLog)));
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ Layer layer = Layer.nameOf(c.getLayer());
+ if (dsls.put(layer, DSL.of(moduleManager, config, c.getDsl())) != null) {
+ throw new UnexpectedException("Layer "+ layer.name() +" has already set a rule.");
Review Comment:
```suggestion
throw new ModuleStartException("Layer " + layer.name() + " has already set a rule.");
```
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968228626
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
LogAnalysisListenerFactory interface is changed for core to add a new layer in create method.
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967713963
##########
docs/en/concepts-and-designs/lal.md:
##########
@@ -7,6 +7,9 @@ segment ID and span ID) and metrics (by generating metrics from the logs and sen
The LAL config files are in YAML format, and are located under directory `lal`. You can
set `log-analyzer/default/lalFiles` in the `application.yml` file or set environment variable `SW_LOG_LAL_FILES` to
activate specific LAL config files.
+## Layer
+Every LAL file is a list of LAL rule, and every LAL rule has a layer property.If a log sent to OAP has the same layer value, this rule will be enable.
+NOTICE: Every layer should only has one rule.
Review Comment:
```suggestion
## Layer
Layer should be declared in the LAL script to represent the analysis scope of the logs.
```
##########
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:
This should be general service layer
##########
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 feel we may not need a default LAL file. You could change this LAL for general service only.
##########
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());
Review Comment:
I think if we can't find suitable layer, we could simply skip the log process.
##########
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:
Pleasw verify whether layer is legal and not empty.
##########
docs/en/concepts-and-designs/lal.md:
##########
@@ -7,6 +7,9 @@ segment ID and span ID) and metrics (by generating metrics from the logs and sen
The LAL config files are in YAML format, and are located under directory `lal`. You can
set `log-analyzer/default/lalFiles` in the `application.yml` file or set environment variable `SW_LOG_LAL_FILES` to
activate specific LAL config files.
+## Layer
+Every LAL file is a list of LAL rule, and every LAL rule has a layer property.If a log sent to OAP has the same layer value, this rule will be enable.
+NOTICE: Every layer should only has one rule.
Review Comment:
Notice the way of describing things in docs. It is not about the code logic, instead it should decribe how end users would use 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] wu-sheng commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968217289
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
> Currently the reason why there is a null in the create method is that the 3 factories share the same interface and the interface is used in the core module of log analysis, if we could separate the sink factory we could just remove the null parameter from the create method.
I feel you are asking the same thing. 3 implementations for one interface. Add one for <2> and <3>, or add one for <1>? I just don't think they are doing the same things, as one for core, the other two for sink. And `null` as a `create` method's parameter seems not right.
I didn't insist on anything relative to which interface and the names. If you want any preference, feel free to propose.
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967778541
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
+ }
}
}
@Override
- public LogAnalysisListener create() {
- return new LogFilterListener(dsls);
+ public LogAnalysisListener create(Layer layer) {
+ if (layer == null) {
+ return null;
Review Comment:
> I have checked it. No other invoker use the return value.
Hi, `listeners` is an `ArrayList`, it doesn't allow null, so if `create` returns `null`, `NullPointerException` will be thrown in the following line
https://github.com/apache/skywalking/blob/81878331e59c435e011eb8c73fe5617201708968/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/LogAnalyzer.java#L79-L80
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#issuecomment-1242880292
Don't worry about that. You just need to focus your changed parts.
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967803267
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
The issue is what I proposed. The create method is adding layer, but those two for sink are not relative.
@yswdqz put a null layer for create method. I feel we should change 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] wu-sheng commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967936175
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/LogAnalyzer.java:
##########
@@ -46,7 +50,20 @@ public void doAnalysis(LogData.Builder builder, Message extraLog) {
log.debug("The log is ignored because the Service name is empty");
return;
}
- createListeners();
+ Layer layer;
+ if ("".equals(builder.getLayer())) {
+ layer = Layer.GENERAL;
+ }
+ else {
+ try {
+ layer = Layer.nameOf(builder.getLayer());
+ } catch (UnexpectedException e) {
+ log.warn("The Layer {} is not found, abandon the log.", builder.getLayer());
+ return;
+ }
+ }
Review Comment:
```suggestion
Layer layer;
if ("".equals(builder.getLayer())) {
layer = Layer.GENERAL;
} else {
try {
layer = Layer.nameOf(builder.getLayer());
} catch (UnexpectedException e) {
log.warn("The Layer {} is not found, abandon the log.", builder.getLayer());
return;
}
}
```
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/LogAnalyzer.java:
##########
@@ -46,7 +50,20 @@ public void doAnalysis(LogData.Builder builder, Message extraLog) {
log.debug("The log is ignored because the Service name is empty");
return;
}
- createListeners();
+ Layer layer;
+ if ("".equals(builder.getLayer())) {
+ layer = Layer.GENERAL;
+ }
+ else {
+ try {
+ layer = Layer.nameOf(builder.getLayer());
+ } catch (UnexpectedException e) {
+ log.warn("The Layer {} is not found, abandon the log.", builder.getLayer());
+ return;
+ }
+ }
Review Comment:
```suggestion
Layer layer;
if ("".equals(builder.getLayer())) {
layer = Layer.GENERAL;
} else {
try {
layer = Layer.nameOf(builder.getLayer());
} catch (UnexpectedException e) {
log.warn("The Layer {} is not found, abandon the log.", builder.getLayer());
return;
}
}
```
--
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] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script
Posted by 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
[GitHub] [skywalking] wu-sheng commented on pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#issuecomment-1242928191
Please check why log e2e are all failure.
--
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] yswdqz commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967797396
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
This listener needn't 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
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r967801569
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -19,61 +19,76 @@
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.UnexpectedException;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
import org.apache.skywalking.oap.server.library.module.ModuleManager;
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- private final List<DSL> dsls;
+ private final DSL dsl;
@Override
public void build() {
- dsls.forEach(dsl -> {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
+ try {
+ if (dsl == null) {
+ log.warn("No rules match, the process will skip.");
+ return;
}
- });
+ 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())
- .extraLog(extraLog)));
+ if (dsl == null) {
+ return null;
+ }
+ dsl.bind(new Binding().log(logData.build()).extraLog(extraLog));
return this;
}
public static class Factory implements LogAnalysisListenerFactory {
- private final List<DSL> dsls;
+ private final Map<Layer, DSL> dsls;
public Factory(final ModuleManager moduleManager, final LogAnalyzerModuleConfig config) throws Exception {
- dsls = new ArrayList<>();
+ dsls = new HashedMap<>();
final List<LALConfig> configList = LALConfigs.load(config.getLalPath(), config.lalFiles())
.stream()
.flatMap(it -> it.getRules().stream())
.collect(Collectors.toList());
for (final LALConfig c : configList) {
- dsls.add(DSL.of(moduleManager, config, c.getDsl()));
+ try {
+ Layer layer = Layer.nameOf(c.getLayer());
+ dsls.put(layer, DSL.of(moduleManager, config, c.getDsl()));
+ } catch (UnexpectedException e) {
+ log.warn("layer not found, will ignore this rule:" + c.getName());
+ }
}
}
@Override
- public LogAnalysisListener create() {
- return new LogFilterListener(dsls);
+ public LogAnalysisListener create(Layer layer) {
+ if (layer == null) {
+ return null;
Review Comment:
> @kezhenxu94 I have tested a null value situation. It will not throw a Exception.
> An ArrayList can store more than one null.
Hey, hosting a `null` value in the list is not a good idea. What's more, did you test the case where the logs will be persisted? The following lines when iterating the array list will throw exception. `listeners.forEach(LogAnalysisListener::build);` Element in `listeners` might be null and calling `null.build()` should throw.
https://github.com/apache/skywalking/blob/166a62612d20e403241aec24b95ceef1c64d898e/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/LogAnalyzer.java#L73-L81
--
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 #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968183634
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
@yswdqz Generally tests passed. Please follow this to separate `LogSinkListener` out of `LogAnalysisListener` implementations. And `LogSinkListener#Factory#create` doesn't require layer as it is generally usable.
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968203537
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
> @yswdqz Generally tests passed. Please follow this to separate `LogSinkListener` out of `LogAnalysisListener` implementations. And `LogSinkListener#Factory#create` doesn't require layer as it is generally usable.
Hi, I’m confused. Currently the reason why there is a `null` in the `create` method is that the 3 factories share the same interface and the interface is used in the core module of log analysis, if we could separate the sink factory we could just remove the null parameter from the create method. Adding a new sink listener factory doesn’t help from what I can tell. Or I misunderstood your request here.
--
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 a diff in pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#discussion_r968222852
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/spec/filter/FilterSpec.java:
##########
@@ -169,7 +169,7 @@ public void sink(@DelegatesTo(SinkSpec.class) final Closure<?> cl) {
final Optional<AtomicReference<Log>> container = BINDING.get().logContainer();
if (container.isPresent()) {
factories.stream()
- .map(LogAnalysisListenerFactory::create)
+ .map(it -> it.create(null))
Review Comment:
> @kezhenxu94 The reason is `LogFilterListener`, it is required to have a `layer` as parameter to create `listener`, which we changed in this PR.
>
> But once we add them, we have to change all others due to this hierarchy tree.
>
> <img alt="image" width="675" src="https://user-images.githubusercontent.com/5441976/189624683-9b7d0368-86cb-4d49-9c32-4788e114f1d7.png">
>
> The only reason I asked for this, is to make `create(null)` disappear.
That’s my point. How to separate the 3 factories? Add a new sink factory and then add the sink factory here?
https://github.com/apache/skywalking/blob/5ba874a49946571506b6cb3c1abbf0d4ce1863bf/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/ILogAnalysisListenerManager.java#L23
```yaml
void addSinkListenerFactory(LogSinkListenerFactory factory);
```
--
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] yswdqz commented on pull request #9593: Add layer property to LAL script
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9593:
URL: https://github.com/apache/skywalking/pull/9593#issuecomment-1243695945
LogSinkListener.Factory's constructor 's LogAnalyzerModuleConfig param maybe also need to remove.
https://github.com/yswdqz/skywalking/blob/lalchange/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/RecordSinkListener.java#L151
https://github.com/yswdqz/skywalking/blob/lalchange/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/TrafficSinkListener.java#L104
--
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