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/10/20 14:13:40 UTC
[GitHub] [skywalking] yswdqz opened a new pull request, #9822: Change the way of loading MAL rules
yswdqz opened a new pull request, #9822:
URL: https://github.com/apache/skywalking/pull/9822
When setting the level for MAL metrics, using the service and instance methods for each metric will generate a lot of redundant code.
We can use MAL's "expSuffix" to avoid it, but because the "expSuffix" required by the instance level metric and the service level metric is different, it will inevitably lead to file splitting (mysql.yaml -> mysql-instance.yaml + mysql-service.yaml )
So I changed the loading method of MAL files to be folder based to facilitate future expansion.
I haven't updated the dicyet because I think maybe somewhere needs to be modified.
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] 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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002411848
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +50,72 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
Review Comment:
Could you build UTs for the new logic? Such as through `loadRules` 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] yswdqz commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002490176
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +50,72 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
Review Comment:
> Could you build UTs for the new logic? Such as through `loadRules` method?
What does `build UTs` mean? Is it extract a 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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003348488
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -55,24 +57,24 @@ public static List<Rule> loadRules(final String path, List<String> enabledRules)
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- final List<String> formedEnabledRules =
- enabledRules
- .stream()
- .map(rule -> {
- if (rule.startsWith("/")) {
- rule = rule.substring(1);
- }
- if (rule.endsWith(".yaml")) {
- return rule;
- } else if (rule.endsWith(".yml")) {
- return rule.substring(0, rule.length() - 2) + "aml";
- } else {
- return rule + ".yaml";
- }
- })
- .collect(Collectors.toList());
+ Map<String, Boolean> formedEnabledRules = enabledRules
Review Comment:
What does this value true/false mean?
--
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] sonatype-lift[bot] commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005223843
##########
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/ResourceUtils.java:
##########
@@ -90,4 +91,8 @@ private static List<File> getDirectoryFilesRecursive(String directoryPath, List<
}
return fileList;
}
+
+ public static Path getPath(String path) {
+ return new File(Objects.requireNonNull(ResourceUtils.class.getClassLoader().getResource(path)).getPath()).toPath();
Review Comment:
*[PATH_TRAVERSAL_IN](https://find-sec-bugs.github.io/bugs.htm#PATH_TRAVERSAL_IN):* This API (java/io/File.<init>(Ljava/lang/String;)V) reads a file whose location might be specified by user input
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=348178577&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=348178577&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=348178577&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=348178577&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=348178577&lift_comment_rating=5) ]
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003358427
##########
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/ResourceUtils.java:
##########
@@ -90,4 +90,13 @@ private static List<File> getDirectoryFilesRecursive(String directoryPath, List<
}
return fileList;
}
+
+ public static File[] getAllPathFiles(String path) throws FileNotFoundException {
Review Comment:
```suggestion
public static File[] list(String path) throws FileNotFoundException {
```
I think we should follow Java-style to use `list`? `getAllPathFiles` seems very tricky.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005435247
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
+ Path relativize = root.relativize(pathPointer);
+ String relativizePath = relativize.toString();
+ relativizePath = relativizePath.substring(0, relativizePath.lastIndexOf("."));
+ return getRulesFromFile(relativizePath, pathPointer);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ List<String> rulesNotFound = formedEnabledRules.keySet().stream()
+ .filter(rule -> !formedEnabledRules.get(rule))
+ .collect(Collectors.toList());
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + rulesNotFound);
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(String ruleName, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ if (fileName.startsWith(".")) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
Review Comment:
Not sure why we need to skip hidden files, but if you insist, please use this
```java
if (!file.isFile() || file.isHidden()) {
return null;
}
```
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
Review Comment:
Why `pathPointer`? What does `pointer` mean?
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
+ Path relativize = root.relativize(pathPointer);
+ String relativizePath = relativize.toString();
+ relativizePath = relativizePath.substring(0, relativizePath.lastIndexOf("."));
+ return getRulesFromFile(relativizePath, pathPointer);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ List<String> rulesNotFound = formedEnabledRules.keySet().stream()
+ .filter(rule -> !formedEnabledRules.get(rule))
+ .collect(Collectors.toList());
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + rulesNotFound);
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(String ruleName, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ if (fileName.startsWith(".")) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
+ Rule rule = new Yaml().loadAs(r, Rule.class);
+ if (rule == null) {
+ return null;
+ }
+ rule.setName(ruleName);
Review Comment:
@yswdqz can you confirm whether rule.name is used anywhere? I locally search and find nowhere use rule.name. If you can confirm rule.name is never used, we may consider remove 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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005714030
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
+ Path relativize = root.relativize(pathPointer);
+ String relativizePath = relativize.toString();
+ relativizePath = relativizePath.substring(0, relativizePath.lastIndexOf("."));
+ return getRulesFromFile(relativizePath, pathPointer);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ List<String> rulesNotFound = formedEnabledRules.keySet().stream()
+ .filter(rule -> !formedEnabledRules.get(rule))
+ .collect(Collectors.toList());
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + rulesNotFound);
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(String ruleName, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ if (fileName.startsWith(".")) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
Review Comment:
We could use Zhenxu's way to do so.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005467754
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
+ Path relativize = root.relativize(pathPointer);
+ String relativizePath = relativize.toString();
+ relativizePath = relativizePath.substring(0, relativizePath.lastIndexOf("."));
+ return getRulesFromFile(relativizePath, pathPointer);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ List<String> rulesNotFound = formedEnabledRules.keySet().stream()
+ .filter(rule -> !formedEnabledRules.get(rule))
+ .collect(Collectors.toList());
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + rulesNotFound);
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(String ruleName, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ if (fileName.startsWith(".")) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
Review Comment:
The hidden file would exist when we set the rule as `/folder/*`.(Such as .DS_store in MacOS) That was the first request.
But with we added `.yml` and `.yaml` to the match pattern, I think hidden file is not an issue.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285799771
What is the question? My point is `/folder` and `folder` have no difference. The we only have one concept, root and current path are the same thing for MAL initialization, no matter in `otel-rules` or others
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002456533
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +50,72 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
Review Comment:
Got 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] yswdqz commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003353374
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -55,24 +57,24 @@ public static List<Rule> loadRules(final String path, List<String> enabledRules)
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- final List<String> formedEnabledRules =
- enabledRules
- .stream()
- .map(rule -> {
- if (rule.startsWith("/")) {
- rule = rule.substring(1);
- }
- if (rule.endsWith(".yaml")) {
- return rule;
- } else if (rule.endsWith(".yml")) {
- return rule.substring(0, rule.length() - 2) + "aml";
- } else {
- return rule + ".yaml";
- }
- })
- .collect(Collectors.toList());
+ Map<String, Boolean> formedEnabledRules = enabledRules
Review Comment:
At first they were false.
If a rule found its configuration file, it change to true.
So that I can judge which enabled rule's file is not exist by map's false 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] wu-sheng commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005299086
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,71 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(path1 -> getRulesFromFile(root.relativize(path1), path1))
Review Comment:
```suggestion
.map(pathPointer -> getRulesFromFile(root.relativize(pathPointer), pathPointer))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1291765747
Codes look good to me overall, just some comments above.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005644008
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
+ Path relativize = root.relativize(pathPointer);
+ String relativizePath = relativize.toString();
+ relativizePath = relativizePath.substring(0, relativizePath.lastIndexOf("."));
+ return getRulesFromFile(relativizePath, pathPointer);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ List<String> rulesNotFound = formedEnabledRules.keySet().stream()
+ .filter(rule -> !formedEnabledRules.get(rule))
+ .collect(Collectors.toList());
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + rulesNotFound);
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(String ruleName, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ if (fileName.startsWith(".")) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
Review Comment:
So should we skip hidden files for now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292937126
> Chances might be you mess up the ui submodule. It is pointed to an earlier version and the OAP failed to start up because of that (protocol mismatch)
Uh sorry. I don't mean UI submodule. What I wanted to say is the query protocol module đ¤Ŗ
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292899672
> @yswdqz CI fails, please fix them.
I don't know why the dead link checker fail, and maybe it cause oap failed to start?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005646636
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
+ Path relativize = root.relativize(pathPointer);
+ String relativizePath = relativize.toString();
+ relativizePath = relativizePath.substring(0, relativizePath.lastIndexOf("."));
+ return getRulesFromFile(relativizePath, pathPointer);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ List<String> rulesNotFound = formedEnabledRules.keySet().stream()
+ .filter(rule -> !formedEnabledRules.get(rule))
+ .collect(Collectors.toList());
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + rulesNotFound);
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(String ruleName, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ if (fileName.startsWith(".")) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
+ Rule rule = new Yaml().loadAs(r, Rule.class);
+ if (rule == null) {
+ return null;
+ }
+ rule.setName(ruleName);
Review Comment:
For now it is not be used.
But maybe we will use it in the future? Such as LAL.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003877631
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
Review Comment:
You are using `lastIndexOf`, so `.asf.yaml` would only get the index of the dot in `.yaml`, isn't 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] yswdqz commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285850607
> Do we really have case for deeper folder? I doubt that.
For now we don't have it.
I understand. We can support it when we really need 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] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1286641110
> Maybe we should replace `/folder/*` as 'folder' so that old configuration can be support. And we can judge file/folder by if there is a '/' in the string
@yswdqz "old configuration can be support", do you mean the current implementation can support old configurations file structure?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003918436
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
Review Comment:
I know we should support both. I mean the way you put in codes is strange. Taking two suffixes should do directly, rather than a rename. In your way, we could(maybe not now) output the wrong file causing confusing.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285766208
> > > One more compatibility we could add is, support ignore suffix(`.yaml`). Then a folder should be `/folder/*`, a file(`folder/mal.yaml`) should be either `folder/mal` or `folder/mal.yaml`. By this, we could support users' local configuration files placed and config in the old way.
> >
> >
> > Maybe we should replace `/folder/*` as 'folder' so that old configuration can be support. And we can judge file/folder by if there is a '/' in the string
>
> That is not good practice. In typical setup, files in the folder should include `*` for files in the folder, and `**` represents files in the current and sub-folders.
I understand. Should we support other pattern? Such as mysql/mysql*.yaml
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003355559
##########
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/ResourceUtils.java:
##########
@@ -90,4 +90,13 @@ private static List<File> getDirectoryFilesRecursive(String directoryPath, List<
}
return fileList;
}
+
+ public static File[] getAllPathFiles(String path) throws FileNotFoundException {
Review Comment:
`getPathFiles` will not return folders
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005228487
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,71 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml") && !rule.endsWith("*")) {
Review Comment:
We can use this to ensure that all listed files are `.yaml` or `.yml`
```suggestion
if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
```
So (2)
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,71 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml") && !rule.endsWith("*")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(path1 -> getRulesFromFile(root.relativize(path1), path1))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(Path relative, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.startsWith(".") || dotIndex == -1 || (!fileName.endsWith(".yml") && !fileName.endsWith(".yaml"))) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
Review Comment:
(2) ... we don't need to check these
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005230527
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,71 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml") && !rule.endsWith("*")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(path1 -> getRulesFromFile(root.relativize(path1), path1))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(Path relative, Path path) {
+ File file = path.toFile();
+ if (!file.isFile()) {
+ return null;
+ }
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.startsWith(".") || dotIndex == -1 || (!fileName.endsWith(".yml") && !fileName.endsWith(".yaml"))) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
Review Comment:
Got 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] yswdqz commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285791284
> @yswdqz you edited my comment and that makes me (and others) confused because I didnât post that comment content and mine is gone.
Sorry that is what I want to say ,I made a mistake
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292791774
@yswdqz CI fails, please fix them.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1001514186
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +49,70 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
+ enabledRules
+ .stream()
+ .map(rule -> rule.endsWith(".yaml") ? rule : rule + ".yaml")
Review Comment:
Got 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 pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285840175
Do we really have case for deeper folder? I doubt that.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002499932
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +50,72 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
Review Comment:
Unit Tests. The path/file pattern mechanism is added newly, and it is better if you could test through correct input and wrong put with predictable results(correct file list or exception or empty list)
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1288019274
About the doc. I only found three places to use the loadRules method (otel-rules, log-mal-rules, envoy-metric-rules). So I think some docs needn't be modified.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285768420
* `/folder/*` should be from the root.
* `folder/*` represents `folder` in the current context
* `folder/file` represents a file or a folder. In our case, `folder` is illegal for our case. So, we treat this as a file, just add a default suffix(`.yaml`) by following we are doing for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285789043
@yswdqz you edited my comment and that makes me (and others) confused because I didnât post that comment content and mine is gone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1287687193
> Could you recheck the document? I think we need some adjustment about the documents of metrics relative configurations
>
>
>
> - OTEL https://skywalking.apache.org/docs/main/next/en/setup/backend/opentelemetry-receiver/
>
> - Meter https://skywalking.apache.org/docs/main/next/en/setup/backend/backend-meter/#configuration-file
>
> - Zabbix https://skywalking.apache.org/docs/main/next/en/setup/backend/backend-zabbix/
>
> - Things to review on PR for telegraf https://github.com/apache/skywalking/pull/9620
>
>
>
> We are mentioning these as rule names, but it seems they are not anymore. At least they should be `rule pattern`, rather than `rule name`. @kezhenxu94 WDYT?
>
> <img width="1292" alt="image" src="https://user-images.githubusercontent.com/5441976/197330076-0039dfad-0bbf-4ced-91ba-9bcebd1b379f.png">
>
>
I think we can simply remove the Rile Name column because there is an existing column named Configuration File and users can compose a pattern to include that file.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285798215
> As I can see in this pull request, every folder only has a single file, even the exception one (k8s folder) we are considering to support activating one of them independently.
I need to change mysql.yaml to mysql-service.yaml and mysql-instance.yaml
Postgresql.yaml also has the same situation. So I send this pr.
The reason of these changes is about half of metrics should be instance level, and other metrics should be service level.
Without this change, we can only add level for each metric.
such as:
https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/otel-rules/apisix.yaml
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1291604039
Some nits, generally this is good.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003878354
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
Review Comment:
Why do we change yml to yaml? These lines seems meaning we support file with `.yml` and `.yaml` suffixes? Why don't we put the codes in that way?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003359368
##########
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/ResourceUtils.java:
##########
@@ -90,4 +90,13 @@ private static List<File> getDirectoryFilesRecursive(String directoryPath, List<
}
return fileList;
}
+
+ public static File[] getAllPathFiles(String path) throws FileNotFoundException {
Review Comment:
Got 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] yswdqz commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003551353
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
Review Comment:
I have concern about it.
For files like `abc` which without `.yaml/.yml`. Method will return null.
For files which with `.yaml/.yml`. If it is a empty file. Method will return null.
For files with `.yaml/.yml`, and it is not a empty file. Method will throw YAML Exception. I have not found good way to resolve it.
Because of we don't support getting files recursively, so if temp files is in a deeper folder, we should not concern.
Hidden files (which start with ".") , loader will treat filename as extension name(.asf.yaml -> extension name is .asf,yaml , not yml/yaml -> ignore).
I will add UTs tommorow about these.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1289976588
Thank youīŧ I'm looking at other open source code similar to this situation and haven't found a suitable one.
Thank you for your help.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285761697
> > > One more compatibility we could add is, support ignore suffix(`.yaml`). Then a folder should be `/folder/*`, a file(`folder/mal.yaml`) should be either `folder/mal` or `folder/mal.yaml`. By this, we could support users' local configuration files placed and config in the old way.
> >
> >
> > Maybe we should replace `/folder/*` as 'folder' so that old configuration can be support. And we can judge file/folder by if there is a '/' in the string
>
> That is not good practice. In typical setup, files in the folder should include `*` for files in the folder, and `**` represents files in the current and sub-folders.
I understand.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285778445
> > I understand. Should we support other pattern? Such as mysql/mysql*.yaml
>
> If you want, we could add a utility in SkyWalking for further general use, but I think we don't have to. Your call.
I agree.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285777095
> * `/folder/*` should be from the root.
Is the `root` means MAL 's root folder?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292926631
> Chances might be you mess up the ui submodule. It is pointed to an earlier version and the OAP failed to start up because of that (protocol mismatch)
Got 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 pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1293129563
<img width="798" alt="image" src="https://user-images.githubusercontent.com/5441976/198223021-0d17ecd0-2d9d-4b1b-8860-1e4448014372.png">
The query proto is reverting not updating actually. Your changes should not touch these protocols, as irrelevant
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1001505680
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +49,70 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
+ enabledRules
+ .stream()
+ .map(rule -> rule.endsWith(".yaml") ? rule : rule + ".yaml")
Review Comment:
let's also take `.yml` into consideration, it's also valid extension name of yaml
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002500815
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +50,72 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
Review Comment:
I understand.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003434032
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
Review Comment:
What would happens if illegal files and legal files are mixed together? And I think there are two cases
1. illegal.yaml is mixed with other normal files, then we should fail in booting. Right?
2. Another case is, we have a hidden file, such as `.abc` is a hidden file in the folder, but it is not a YAML file.
Such as in a macOS, you could have several hidden files in every folder, **.DS_Store**.
<img width="675" alt="image" src="https://user-images.githubusercontent.com/5441976/197560595-8d3ccbef-6377-4dc4-a268-baed08c6304c.png">
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002458358
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +50,72 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
+ enabledRules
+ .stream()
+ .map(rule -> {
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
+ }
+ })
+ .collect(Collectors.toList());
+
return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ }
+
+ private static Rule getRulesFromFile(List<String> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
+ if (directory != null) {
+ fileName = directory.getName() + "/" + ruleName;
+ if (!formedEnabledRules.contains(fileName) && !formedEnabledRules.contains(directory.getName() + "/*.yaml")) {
+ return null;
+ }
+ } else {
+ if (!formedEnabledRules.contains(fileName)) {
+ return null;
}
+ }
+
+ Rule rule = new Yaml().loadAs(r, Rule.class);
+ if (rule == null) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
+ rule.setName(ruleName);
+ return rule;
+ } catch (IOException e) {
+ LOG.debug("Reading file {} failed", file, e);
Review Comment:
This is a fatal exception and we should propagate to the caller, and stop the OAP from starting
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1288022749
I think we don't have to move so many real otel YAML files into the tests. The test files should be as simple as possible. Unless you are going to verify some expressions, your tests don't like so
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003350251
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -55,24 +57,24 @@ public static List<Rule> loadRules(final String path, List<String> enabledRules)
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- final List<String> formedEnabledRules =
- enabledRules
- .stream()
- .map(rule -> {
- if (rule.startsWith("/")) {
- rule = rule.substring(1);
- }
- if (rule.endsWith(".yaml")) {
- return rule;
- } else if (rule.endsWith(".yml")) {
- return rule.substring(0, rule.length() - 2) + "aml";
- } else {
- return rule + ".yaml";
- }
- })
- .collect(Collectors.toList());
+ Map<String, Boolean> formedEnabledRules = enabledRules
Review Comment:
This is used to determine whether an enabled rule does not have a corresponding configuration file
--
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] sonatype-lift[bot] commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005277716
##########
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/ResourceUtils.java:
##########
@@ -90,4 +91,8 @@ private static List<File> getDirectoryFilesRecursive(String directoryPath, List<
}
return fileList;
}
+
+ public static Path getPath(String path) {
+ return new File(Objects.requireNonNull(ResourceUtils.class.getClassLoader().getResource(path)).getPath()).toPath();
Review Comment:
I've recorded this as ignored for this pull request.
If you change your mind, just comment `@sonatype-lift unignore`.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1291610868
Got it , I will fix 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 merged pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #9822:
URL: https://github.com/apache/skywalking/pull/9822
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285670208
> One thing I think we should be concerned about is: such as k8s, this way will enable all metrics at the same time and no other choice. Some metrics need access to the API server. If that can be accepted, I'm OK with this.
Maybe we can set k8s as two folders?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285758771
> > One more compatibility we could add is, support ignore suffix(`.yaml`). Then a folder should be `/folder/*`, a file(`folder/mal.yaml`) should be either `folder/mal` or `folder/mal.yaml`. By this, we could support users' local configuration files placed and config in the old way.
>
> Maybe we should replace `/folder/*` as 'folder' so that old configuration can be support. And we can judge file/folder by if there is a '/' in the string
That is not good practice. In typical setup, files in the folder should include `*` for files in the folder, and `**` represents files in the current and sub-folders.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285776883
> I understand. Should we support other pattern? Such as mysql/mysql*.yaml
If you want, we could add a utility in SkyWalking for further general use, but I think we don't have to. Your call.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285779689
> > * `/folder/*` should be from the root.
>
> Is the `root` means MAL 's root folder?
Actually, we don't have the root. Because we have declared all files should be `otel-rules`, the root and current context are the same.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285671509
@yswdqz Do you think is it better to support the path pattern as well as use a file name? Such as `k8s/*.yaml` or `apisix/apisix.yaml`
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1286635060
> > One more compatibility we could add is, support ignore suffix(`.yaml`). Then a folder should be `/folder/*`, a file(`folder/mal.yaml`) should be either `folder/mal` or `folder/mal.yaml`. By this, we could support users' local configuration files placed and config in the old way.
>
> Maybe we should replace `/folder/*` as 'folder' so that old configuration can be support. And we can judge file/folder by if there is a '/' in the string
@kezhenxu94
Please see this. I thought about that too
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003350833
##########
docs/en/changes/changes.md:
##########
@@ -68,6 +68,7 @@
* Add new config initialization mechanism of module provider. This is a ModuleManager lib kernel level change.
* [**Breaking Change**] Support new records query protocol, rename the column named `service_id` to `entity_id` for support difference entity.
Please re-create `top_n_database_statement` index/table.
+* [**Breaking Change**] Change the way of loading MAL rules(support pattern).
Review Comment:
```suggestion
* [**Breaking Change**] Change the way of loading MAL rules(support pattern).
* Move k8s relative MAL files into `/otel-rules/k8s`.
```
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003443304
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
Review Comment:
I think we at least should ignore hidden files?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292940473
If you have synced UI, you need to update change logs accordingly.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285680505
> @yswdqz Do you think is it better to support the path pattern as well as use a file name? Such as `k8s/*.yaml` or `apisix/apisix.yaml`
That's a really good idea!
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003551353
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
Review Comment:
I have concern about it.
For files like `abc` which without `.yaml/.yml`. Method will return null.
For files which with `.yaml/.yml`. If it is a empty file. Method will return null.
For files with `.yaml/.yml`, and it is not a empty file. Method will throw YAML Exception. I have not found good way to resolve it.
Because of we don't support getting files recursively, so if temp files is in a deeper folder, we should not concern.
Tomorrow I will ignore files start with ".".
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005297449
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,71 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(path1 -> getRulesFromFile(root.relativize(path1), path1))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
Review Comment:
I feel you should only put keys with `formedEnabledRules#value == true` 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 pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292918517
Chances might be you mess up the ui submodule. It was pointed to an earlier version and the OAP failed to start up because of that (protocol mismatch)
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1293027837
You don't have to. If you do, update the UI part changelog, according to the commits between before and after commits.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1001505019
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +49,70 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
+ enabledRules
+ .stream()
+ .map(rule -> rule.endsWith(".yaml") ? rule : rule + ".yaml")
+ .map(rule -> rule.startsWith("/") ? rule.substring(1) : rule)
+ .collect(Collectors.toList());
+
return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
+ fileName = f.getName() + '/' + fileName;
+ if (!formedEnabledRules.contains(fileName) && !formedEnabledRules.contains(f.getName() + "/*.yaml")) {
+ return null;
+ }
+ Rule rule = new Yaml().loadAs(r, Rule.class);
+ if (rule == null) {
+ return null;
+ }
+ rule.setName(ruleName);
+ return rule;
+ } catch (IOException e) {
+ LOG.debug("Reading file {} failed", f, e);
+ }
+ return null;
+ });
+ } else {
+ try (Reader r = new FileReader(f)) {
+ String fileName = f.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
+ if (!formedEnabledRules.contains(fileName)) {
+ return null;
+ }
+ Rule rule = new Yaml().loadAs(r, Rule.class);
+ if (rule == null) {
+ return null;
+ }
+ rule.setName(ruleName);
+ return Stream.of(rule);
+ } catch (IOException e) {
+ LOG.debug("Reading file {} failed", f, e);
+ }
Review Comment:
Can you extract this to a method and share it with the lines above in the `if` statement?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1286645052
> > > One more compatibility we could add is, support ignore suffix(`.yaml`). Then a folder should be `/folder/*`, a file(`folder/mal.yaml`) should be either `folder/mal` or `folder/mal.yaml`. By this, we could support users' local configuration files placed and config in the old way.
> >
> >
> > Maybe we should replace `/folder/*` as 'folder' so that old configuration can be support. And we can judge file/folder by if there is a '/' in the string
>
> That is not good practice. In typical setup, files in the folder should include `*` for files in the folder, and `**` represents files in the current and sub-folders.
@kezhenxu94
Sorry I have not said clear.
My idea was rejected 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] wu-sheng commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1287684326
Could you recheck the document? I think we need some adjustment about the documents of metrics relative configurations
- OTEL https://skywalking.apache.org/docs/main/next/en/setup/backend/opentelemetry-receiver/
- Meter https://skywalking.apache.org/docs/main/next/en/setup/backend/backend-meter/#configuration-file
- Zabbix https://skywalking.apache.org/docs/main/next/en/setup/backend/backend-zabbix/
- Things to review on PR for telegraf https://github.com/apache/skywalking/pull/9620
We are mentioning these as rule names, but it seems they are not anymore. At least they should be `rule pattern`, rather than `rule name`. @kezhenxu94 WDYT?
<img width="1292" alt="image" src="https://user-images.githubusercontent.com/5441976/197330076-0039dfad-0bbf-4ced-91ba-9bcebd1b379f.png">
--
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] wankai123 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wankai123 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285664152
One thing I think we should be concerned about is: such as k8s, this way will enable all metrics at the same time and no other choice. Some metrics need access to the API server. If that can be accepted, I'm OK with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] yswdqz commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285736943
> One more compatibility we could add is, support ignore suffix(`.yaml`). Then a folder should be `/folder/*`, a file(`folder/mal.yaml`) should be either `folder/mal` or `folder/mal.yaml`. By this, we could support users' local configuration files placed and config in the old way.
Maybe we should replace `/folder/*` as 'folder' so that old configuration can be support.
And we can judge file/folder by if there is a '/' in the string
--
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] sonatype-lift[bot] commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285684021
:warning: **6 God Classes** were detected by Lift in this project. [Visit the Lift web console](https://lift.sonatype.com/results/github.com/apache/skywalking/01GFTVW72SEX8QPCPSC7SSG83V?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache\%20skywalking) for more details.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285713973
> > @yswdqz Do you think is it better to support the path pattern as well as use a file name? Such as `k8s/*.yaml` or `apisix/apisix.yaml`
>
> That's a really good idea!
One more compatibility we could add is, support ignore suffix(`.yaml`). Then a folder should be `/folder/*`, a file(`folder/mal.yaml`) should be either `folder/mal` or `folder/mal.yaml`.
By this, we could support users' local configuration files placed and config in the old way.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285786256
> As I can see in this pull request, every folder only has a single file, even the exception one (k8s folder) we are considering to support activating one of them independently.
Actually, there was an off-list discussion from https://github.com/apache/skywalking/issues/9762
MySQL would become multiple files as instance files would be added from MAL analysis.
I think the main reason for this, we are setting entity statements in expSuffix. So, once we need service and instance levels metrics at the same time, we usually use two files.
> expSuffix: tag({tags -> tags.cluster = 'istio-ctrl::' + tags.cluster}).service(['cluster', 'app'], Layer.MESH_CP)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285782918
As I can see in this pull request, every folder only has a single file, even the exception one (k8s folder) we are considering to support activating one of them independently.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005277618
##########
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/ResourceUtils.java:
##########
@@ -90,4 +91,8 @@ private static List<File> getDirectoryFilesRecursive(String directoryPath, List<
}
return fileList;
}
+
+ public static Path getPath(String path) {
+ return new File(Objects.requireNonNull(ResourceUtils.class.getClassLoader().getResource(path)).getPath()).toPath();
Review Comment:
@sonatype-lift ignore
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005468641
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
Review Comment:
The `path` variable is used, it was named as `path`. So, I suggest adding a point suffix just to separate the name in closure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292913289
> > @yswdqz CI fails, please fix them.
>
>
>
> I don't know why the dead link checker fail, and maybe it cause oap failed to start?
Dead link checker won't block OAP from starting.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005395441
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,76 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(pathPointer -> {
+ Path relativize = root.relativize(pathPointer);
+ String relativizePath = relativize.toString();
+ relativizePath = relativizePath.substring(0, relativizePath.lastIndexOf("."));
+ return getRulesFromFile(relativizePath, pathPointer);
Review Comment:
```suggestion
// Use relativized file path without suffix as the rule name.
String ruleName = root.relativize(pathPointer).toString().substring(0, relativizePath.lastIndexOf("."));
return getRulesFromFile(ruleName, pathPointer);
```
I think this is better?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003912951
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
Review Comment:
My fault. I forget 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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1005300242
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -40,36 +49,71 @@
public class Rules {
private static final Logger LOG = LoggerFactory.getLogger(Rule.class);
- public static List<Rule> loadRules(final String path) throws ModuleStartException {
+ public static List<Rule> loadRules(final String path) throws IOException {
return loadRules(path, Collections.emptyList());
}
- public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
- File[] rules;
- try {
- rules = ResourceUtils.getPathFiles(path);
- } catch (FileNotFoundException e) {
- throw new ModuleStartException("Load fetcher rules failed", e);
- }
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ public static List<Rule> loadRules(final String path, List<String> enabledRules) throws IOException {
+
+ final Path root = ResourceUtils.getPath(path);
+
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (!rule.endsWith(".yaml") && !rule.endsWith(".yml")) {
+ return rule + "{.yaml,.yml}";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
- }
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+ List<Rule> rules;
+ try (Stream<Path> stream = Files.walk(root)) {
+ rules = stream
+ .filter(it -> formedEnabledRules.keySet().stream()
+ .anyMatch(rule -> {
+ boolean matches = FileSystems.getDefault().getPathMatcher("glob:" + rule)
+ .matches(root.relativize(it));
+ if (matches) {
+ formedEnabledRules.put(rule, true);
+ }
+ return matches;
+ }))
+ .map(path1 -> getRulesFromFile(root.relativize(path1), path1))
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList()) ;
+ }
+
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return rules;
+ }
+
+ private static Rule getRulesFromFile(Path relative, Path path) {
Review Comment:
```suggestion
private static Rule getRulesFromFile(String ruleName, Path path) {
```
I think the caller should build the ruleName, rather than hide the logic inside this method with two Paths as parameters.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292937734
> Uh sorry. I don't mean UI submodule. What I wanted to say is the query protocol module đ¤Ŗ
Got 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] yswdqz commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003915689
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
Review Comment:
For this, we support .yml.
And this will cause a problem : enabledRule: mysql means mysql.yml or mysql.yaml ? So I just change yml to yaml to support both situation. The only problem with such code is that if there are both mysql.yml and mysql.yaml, there is no guarantee which one will be loaded.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003354058
##########
docs/en/changes/changes.md:
##########
@@ -68,6 +68,7 @@
* Add new config initialization mechanism of module provider. This is a ModuleManager lib kernel level change.
* [**Breaking Change**] Support new records query protocol, rename the column named `service_id` to `entity_id` for support difference entity.
Please re-create `top_n_database_statement` index/table.
+* [**Breaking Change**] Change the way of loading MAL rules(support pattern).
Review Comment:
Got 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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003354190
##########
oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/ResourceUtils.java:
##########
@@ -90,4 +90,13 @@ private static List<File> getDirectoryFilesRecursive(String directoryPath, List<
}
return fileList;
}
+
+ public static File[] getAllPathFiles(String path) throws FileNotFoundException {
Review Comment:
What is the difference from existing `getPathFiles` 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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002651726
##########
oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/RuleLoaderTest.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.meter.analyzer.dsl;
+
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.meter.analyzer.prometheus.rule.Rule;
+import org.apache.skywalking.oap.meter.analyzer.prometheus.rule.Rules;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.UUID;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.fail;
+
+@Slf4j
+@RunWith(Parameterized.class)
+public class RuleLoaderTest {
+ @Parameterized.Parameter
+ public List<String> enabledRule;
+
+ @Parameterized.Parameter(1)
+ public int rulesNumber;
+
+ @Parameterized.Parameter(2)
+ public boolean isThrow;
+
+ @Parameterized.Parameters(name = "{index}: {0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ // abc/abc.yml is a not well-formed yml file.
+ {Arrays.asList("abc/*"), 0, true},
Review Comment:
The good test mock samples should be
- /illegal-yaml/test.yml
- /test-folder/case1.yml
- single-file-case.yml
##########
oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/RuleLoaderTest.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.meter.analyzer.dsl;
+
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.meter.analyzer.prometheus.rule.Rule;
+import org.apache.skywalking.oap.meter.analyzer.prometheus.rule.Rules;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.UUID;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.fail;
+
+@Slf4j
+@RunWith(Parameterized.class)
+public class RuleLoaderTest {
+ @Parameterized.Parameter
+ public List<String> enabledRule;
+
+ @Parameterized.Parameter(1)
+ public int rulesNumber;
+
+ @Parameterized.Parameter(2)
+ public boolean isThrow;
+
+ @Parameterized.Parameters(name = "{index}: {0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ // abc/abc.yml is a not well-formed yml file.
+ {Arrays.asList("abc/*"), 0, true},
+ {Arrays.asList("k8s/*"), 4, false},
+ {Arrays.asList("/k8s/*.yaml"), 4, false},
+ {Arrays.asList("/k8s/*.yml"), 4, false},
+ {Arrays.asList("k8s/*.yaml"), 4, false},
+ {Arrays.asList("k8s/*.yml"), 4, false},
+ {Arrays.asList("k8s/k8s-cluster.yml"), 1, false},
+ {Arrays.asList("/k8s/k8s-cluster.yml"), 1, false},
+ {Arrays.asList("/k8s/k8s-cluster.yaml"), 1, false},
+ {Arrays.asList("k8s/k8s-cluster.yaml"), 1, false},
+ {Arrays.asList("k8s/k8s-cluster"), 1, false},
+ {Arrays.asList("oap.yaml"), 1, false},
+ {Arrays.asList("oap.yml"), 1, false},
+ {Arrays.asList("oap"), 1, false},
+ {Arrays.asList("/oap.yaml"), 1, false},
+ {Arrays.asList("/oap.yml"), 1, false},
+ {Arrays.asList("/oap.yml", "/k8s/*"), 5, false},
+ //if enabledRule not found, will not fail but will not load any rules
+ {Arrays.asList(UUID.randomUUID().toString()), 0, false},
Review Comment:
I think this should through an exception rather than a `false` return.
##########
oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/RuleLoaderTest.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.meter.analyzer.dsl;
+
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.meter.analyzer.prometheus.rule.Rule;
+import org.apache.skywalking.oap.meter.analyzer.prometheus.rule.Rules;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.UUID;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.fail;
+
+@Slf4j
+@RunWith(Parameterized.class)
+public class RuleLoaderTest {
+ @Parameterized.Parameter
+ public List<String> enabledRule;
+
+ @Parameterized.Parameter(1)
+ public int rulesNumber;
+
+ @Parameterized.Parameter(2)
+ public boolean isThrow;
+
+ @Parameterized.Parameters(name = "{index}: {0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] {
+ // abc/abc.yml is a not well-formed yml file.
+ {Arrays.asList("abc/*"), 0, true},
+ {Arrays.asList("k8s/*"), 4, false},
+ {Arrays.asList("/k8s/*.yaml"), 4, false},
+ {Arrays.asList("/k8s/*.yml"), 4, false},
+ {Arrays.asList("k8s/*.yaml"), 4, false},
+ {Arrays.asList("k8s/*.yml"), 4, false},
+ {Arrays.asList("k8s/k8s-cluster.yml"), 1, false},
+ {Arrays.asList("/k8s/k8s-cluster.yml"), 1, false},
+ {Arrays.asList("/k8s/k8s-cluster.yaml"), 1, false},
+ {Arrays.asList("k8s/k8s-cluster.yaml"), 1, false},
+ {Arrays.asList("k8s/k8s-cluster"), 1, false},
+ {Arrays.asList("oap.yaml"), 1, false},
+ {Arrays.asList("oap.yml"), 1, false},
+ {Arrays.asList("oap"), 1, false},
+ {Arrays.asList("/oap.yaml"), 1, false},
+ {Arrays.asList("/oap.yml"), 1, false},
+ {Arrays.asList("/oap.yml", "/k8s/*"), 5, false},
+ //if enabledRule not found, will not fail but will not load any rules
+ {Arrays.asList(UUID.randomUUID().toString()), 0, false},
+ });
+ }
+
+ @Test
+ public void test() {
+ List<Rule> rules = null;
+ try {
+ rules = Rules.loadRules("otel-rules", enabledRule);
+ } catch (Exception e) {
+ if (isThrow) {
+ return;
+ }
+ fail("load rules failed");
+ }
Review Comment:
Learn how to wrong predicable exceptions in UT
```java
@Test(expected = IllegalArgumentException.class)
```
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003347081
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -55,24 +57,24 @@ public static List<Rule> loadRules(final String path, List<String> enabledRules)
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- final List<String> formedEnabledRules =
- enabledRules
- .stream()
- .map(rule -> {
- if (rule.startsWith("/")) {
- rule = rule.substring(1);
- }
- if (rule.endsWith(".yaml")) {
- return rule;
- } else if (rule.endsWith(".yml")) {
- return rule.substring(0, rule.length() - 2) + "aml";
- } else {
- return rule + ".yaml";
- }
- })
- .collect(Collectors.toList());
+ Map<String, Boolean> formedEnabledRules = enabledRules
Review Comment:
Map<String, Boolean> means a Set<String>?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1003923782
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +53,79 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.list(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
- return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ Map<String, Boolean> formedEnabledRules = enabledRules
+ .stream()
+ .map(rule -> {
+ rule = rule.trim();
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .collect(Collectors.toMap(rule -> rule, $ -> false));
+
+ List<Rule> result = Arrays.stream(rules)
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
+ }
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ if (formedEnabledRules.containsValue(false)) {
+ throw new UnexpectedException("Some configuration files of enabled rules are not found, enabled rules: " + formedEnabledRules.keySet());
+ }
+ return result;
+ }
+
+ private static Rule getRulesFromFile(Map<String, Boolean> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
Review Comment:
I understand, I will try it later.
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1292948139
> If you have synced UI, you need to update change logs accordingly.
How can I update logs about?
By the way, I have not change codes about ui, should I sync UI ?
--
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] sonatype-lift[bot] commented on a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1000745515
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +47,36 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getPathDirectories(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ .filter(dir -> enabledRules.contains(dir.getName()))
+ .map(File::listFiles)
+ .filter(Objects::nonNull)
+ .flatMap(Arrays::stream)
+ .filter(File::isFile)
+ .map(f -> {
+ try (Reader r = new FileReader(f)) {
Review Comment:
*[DefaultCharset](https://errorprone.info/bugpattern/DefaultCharset):* Implicit use of the platform default charset, which can result in differing behaviour between JVM executions or incorrect behavior if the encoding of the data source doesn't match expectations.
---
```suggestion
try (Reader r = Files.newBufferedReader(f.toPath(), Charset.defaultCharset())) {
```
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=346794765&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=346794765&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=346794765&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=346794765&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=346794765&lift_comment_rating=5) ]
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285630390
@kezhenxu94 @wankai123 What do you think to do this breaking change?
--
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 #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285821372
> What is the question? My point is `/folder` and `folder` have no difference. The we only have one concept, root and current path are the same thing for MAL initialization, no matter in `otel-rules` or others
I understand now.
So we should support:
- `mysql/mysql-instance.yaml` : for file
- `mysql/mysql-instance` : the same as `mysql/mysql-instance.yaml`
- `/mysql/mysql-instance` : the same as `mysql/mysql-instance.yaml`
- `/mysql/mysql-instance.yaml` : the same as `mysql/mysql-instance.yaml`
- `/mysql/*.yaml` : for folder
- `/mysql/*` : the same as `/mysql/*.yaml`
- `mysql/*.yaml` : the same as `/mysql/*.yaml`
- `mysql/*` : the same as `/mysql/*.yaml`
Is it right? And maybe we should support `**` , too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#issuecomment-1285779879
Hi. What's the idea / use case here to move the yaml files into a folder, I'm thinking this just leads to "one file per folder" eventually and doesn't make much sense at 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 a diff in pull request #9822: Change the way of loading MAL rules
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #9822:
URL: https://github.com/apache/skywalking/pull/9822#discussion_r1002459444
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/prometheus/rule/Rules.java:
##########
@@ -47,29 +50,72 @@ public static List<Rule> loadRules(final String path) throws ModuleStartExceptio
public static List<Rule> loadRules(final String path, List<String> enabledRules) throws ModuleStartException {
File[] rules;
try {
- rules = ResourceUtils.getPathFiles(path);
+ rules = ResourceUtils.getAllPathFiles(path);
} catch (FileNotFoundException e) {
throw new ModuleStartException("Load fetcher rules failed", e);
}
+ final List<String> formedEnabledRules =
+ enabledRules
+ .stream()
+ .map(rule -> {
+ if (rule.startsWith("/")) {
+ rule = rule.substring(1);
+ }
+ if (rule.endsWith(".yaml")) {
+ return rule;
+ } else if (rule.endsWith(".yml")) {
+ return rule.substring(0, rule.length() - 2) + "aml";
+ } else {
+ return rule + ".yaml";
+ }
+ })
+ .collect(Collectors.toList());
+
return Arrays.stream(rules)
- .filter(File::isFile)
- .map(f -> {
- try (Reader r = new FileReader(f)) {
- String fileName = f.getName();
- int dotIndex = fileName.lastIndexOf('.');
- fileName = (dotIndex == -1) ? fileName : fileName.substring(0, dotIndex);
- if (!enabledRules.contains(fileName)) {
- return null;
+ .flatMap(f -> {
+ if (f.isDirectory()) {
+ return Arrays.stream(Objects.requireNonNull(f.listFiles()))
+ .filter(File::isFile)
+ .map(file -> getRulesFromFile(formedEnabledRules, f, file));
+ } else {
+ return Stream.of(getRulesFromFile(formedEnabledRules, null, f));
}
- Rule rule = new Yaml().loadAs(r, Rule.class);
- rule.setName(fileName);
- return rule;
- } catch (IOException e) {
- LOG.debug("Reading file {} failed", f, e);
+ })
+ .filter(Objects::nonNull)
+ .collect(Collectors.toList());
+ }
+
+ private static Rule getRulesFromFile(List<String> formedEnabledRules, File directory, File file) {
+ try (Reader r = new FileReader(file)) {
+ String fileName = file.getName();
+ int dotIndex = fileName.lastIndexOf('.');
+ if (fileName.endsWith(".yml")) {
+ fileName = fileName.substring(0, fileName.length() - 2) + "aml";
+ }
+ if (dotIndex == -1 || !"yaml".equals(fileName.substring(dotIndex + 1))) {
+ return null;
+ }
+ String ruleName = fileName.substring(0, dotIndex);
+ if (directory != null) {
+ fileName = directory.getName() + "/" + ruleName;
+ if (!formedEnabledRules.contains(fileName) && !formedEnabledRules.contains(directory.getName() + "/*.yaml")) {
+ return null;
+ }
+ } else {
+ if (!formedEnabledRules.contains(fileName)) {
+ return null;
}
+ }
+
+ Rule rule = new Yaml().loadAs(r, Rule.class);
+ if (rule == null) {
return null;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList());
+ }
+ rule.setName(ruleName);
+ return rule;
+ } catch (IOException e) {
+ LOG.debug("Reading file {} failed", file, e);
Review Comment:
Got 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