Copilot commented on code in PR #13697:
URL: https://github.com/apache/skywalking/pull/13697#discussion_r2787219367
##########
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/EnvoyMetricReceiverConfig.java:
##########
@@ -87,7 +89,12 @@ public List<String> getAlsTCPAnalysis() {
public List<Rule> rules() throws ModuleStartException {
try {
- return Rules.loadRules("envoy-metrics-rules",
Arrays.asList("envoy", "envoy-svc-relation"));
+ final List<String> enabledRules =
+ Splitter.on(",")
+ .omitEmptyStrings()
+ .trimResults()
+ .splitToList(enabledEnvoyMetricsRules);
Review Comment:
`enabledEnvoyMetricsRules` can be null when users upgrade with an older
`application.yml` that doesn't include this new setting.
`Splitter.splitToList(enabledEnvoyMetricsRules)` will then throw an NPE during
startup (when `acceptMetricsService` is enabled). Consider keeping backward
compatibility by defaulting to the previous hard-coded rules
(`envoy,envoy-svc-relation`) when `enabledEnvoyMetricsRules == null`, while
still allowing an explicit empty string to disable all rules.
```suggestion
// Keep backward compatibility: if the config is absent (null),
use the previous default rules.
// An explicit empty string ("") still disables all rules.
final String rulesConfig = enabledEnvoyMetricsRules == null
? "envoy,envoy-svc-relation"
: enabledEnvoyMetricsRules;
final List<String> enabledRules =
Splitter.on(",")
.omitEmptyStrings()
.trimResults()
.splitToList(rulesConfig);
```
##########
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/EnvoyMetricReceiverConfig.java:
##########
@@ -87,7 +89,12 @@ public List<String> getAlsTCPAnalysis() {
public List<Rule> rules() throws ModuleStartException {
try {
- return Rules.loadRules("envoy-metrics-rules",
Arrays.asList("envoy", "envoy-svc-relation"));
+ final List<String> enabledRules =
+ Splitter.on(",")
+ .omitEmptyStrings()
+ .trimResults()
+ .splitToList(enabledEnvoyMetricsRules);
+ return Rules.loadRules("envoy-metrics-rules", enabledRules);
Review Comment:
The new rule-selection behavior (parsing `enabledEnvoyMetricsRules` and
passing it to `Rules.loadRules`) isn't covered by tests in this module. Adding
a small unit test for `rules()` covering (1) null/missing config uses defaults,
(2) empty string disables all rules, and (3) custom comma-separated values load
expected files would help prevent regressions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
