An unexpected dot is added when exp is a pure metric name and expPref…

…ix != null (apache#10142) > The final expression was (metric_name(exp).expPrefix)`.`(this is the extra dot) The correct one should be (metric_name(exp).expPrefix)
renovamens · Dec 11, 2022 · 488737e · 488737e
1 parent 3229a5c
commit 488737e
Show file tree

Hide file tree

Showing 3 changed files with 270 additions and 17 deletions.
diff --git a/docs/en/changes/changes.md b/docs/en/changes/changes.md
@@ -8,6 +8,7 @@
 * Adds Micrometer as a new component.(ID=141)
 * Refactor session cache in MetricsPersistentWorker.
 * Cache enhancement - don't read new metrics from database in minute dimensionality.
+* Fix MAL exp combine error when only contains one segment and expPrefix not blank
 
 ```
     // When

diff --git a/.../meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/MetricConvert.java b/.../meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/MetricConvert.java
@@ -57,27 +57,43 @@ public MetricConvert(MetricRuleConfig rule, MeterSystem service) {
         if (StringUtils.isNotEmpty(rule.getInitExp())) {
             handleInitExp(rule.getInitExp());
         }
-
         this.analyzers = rule.getMetricsRules().stream().map(
-            r -> {
-                String exp = r.getExp();
-                if (!Strings.isNullOrEmpty(rule.getExpPrefix())) {
-                    exp = String.format("(%s.%s).%s", StringUtils.substringBefore(exp, "."), rule.getExpPrefix(),
-                            StringUtils.substringAfter(exp, "."));
-                }
-                if (!Strings.isNullOrEmpty(rule.getExpSuffix())) {
-                    exp = String.format("(%s).%s", exp, rule.getExpSuffix());
-                }
-                return Analyzer.build(
-                    formatMetricName(rule, r.getName()),
-                    rule.getFilter(),
-                    exp,
-                    service
-                );
-            }
+            r -> buildAnalyzer(
+                formatMetricName(rule, r.getName()),
+                rule.getFilter(),
+                formatExp(rule.getExpPrefix(), rule.getExpSuffix(), r.getExp()),
+                service
+            )
         ).collect(toList());
     }
 
+    Analyzer buildAnalyzer(final String metricsName,
+                           final String filter,
+                           final String exp,
+                           final MeterSystem service) {
+        return Analyzer.build(
+            metricsName,
+            filter,
+            exp,
+            service
+        );
+    }
+
+    private String formatExp(final String expPrefix, String expSuffix, String exp) {
+        String ret = exp;
+        if (!Strings.isNullOrEmpty(expPrefix)) {
+            ret = String.format("(%s.%s)", StringUtils.substringBefore(exp, "."), expPrefix);
+            final String after = StringUtils.substringAfter(exp, ".");
+            if (!Strings.isNullOrEmpty(after)) {
+                ret = String.format("(%s.%s)", ret, after);
+            }
+        }
+        if (!Strings.isNullOrEmpty(expSuffix)) {
+            ret = String.format("(%s).%s", ret, expSuffix);
+        }
+        return ret;
+    }
+
     /**
      * toMeter transforms {@link SampleFamily} collection  to meter-system metrics.
      *

diff --git a/...er-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/MetricConvertTest.java b/...er-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/MetricConvertTest.java
@@ -0,0 +1,236 @@
+/*
+ * 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;
+
+import java.util.Arrays;
+import java.util.List;
+import lombok.AllArgsConstructor;
+import lombok.Getter;
+import org.apache.skywalking.oap.server.core.analysis.meter.MeterSystem;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class MetricConvertTest {
+
+    @Test
+    public void testOneLevelExp() {
+        MockMetricRuleConfig mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            "tag({tags -> tags.service_name = 2})",
+            "tag({tags -> tags.service_name = 1})",
+            "{ tags -> tags.job_name == 'apisix-monitoring' }",
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections"
+            )),
+            null
+        );
+        MockMetricConvert metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals("filter", "{ tags -> tags.job_name == 'apisix-monitoring' }", metricConvert.filter);
+        Assert.assertEquals(
+            "exp",
+            "((apisix_nginx_http_current_connections.tag({tags -> tags.service_name = 1}))).tag({tags -> tags.service_name = 2})",
+            metricConvert.exp
+        );
+
+        // expSuffix is null
+        mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            null,
+            "tag({tags -> tags.service_name = 1})",
+            null,
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections"
+            )),
+            null
+        );
+        metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals(
+            "exp", "(apisix_nginx_http_current_connections.tag({tags -> tags.service_name = 1}))",
+            metricConvert.exp
+        );
+
+        // expPrefix is null
+        mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            "tag({tags -> tags.service_name = 2})",
+            null,
+            null,
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections"
+            )),
+            null
+        );
+        metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals(
+            "exp", "(apisix_nginx_http_current_connections).tag({tags -> tags.service_name = 2})",
+            metricConvert.exp
+        );
+
+        // expPrefix and expSuffix is null
+        mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            null,
+            null,
+            null,
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections"
+            )),
+            null
+        );
+        metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals(
+            "exp", "apisix_nginx_http_current_connections",
+            metricConvert.exp
+        );
+
+    }
+
+    @Test
+    public void testMultipleLevelExp() {
+        MockMetricRuleConfig mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            "tag({tags -> tags.service_name = 2})",
+            "tag({tags -> tags.service_name = 1})",
+            "{ tags -> tags.job_name == 'apisix-monitoring' }",
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections.sum(['a'])"
+            )),
+            null
+        );
+        MockMetricConvert metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals("filter", "{ tags -> tags.job_name == 'apisix-monitoring' }", metricConvert.filter);
+        Assert.assertEquals(
+            "exp",
+            "(((apisix_nginx_http_current_connections.tag({tags -> tags.service_name = 1})).sum(['a']))).tag({tags -> tags.service_name = 2})",
+            metricConvert.exp
+        );
+
+        // expSuffix is null
+        mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            null,
+            "tag({tags -> tags.service_name = 1})",
+            null,
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections.downsampling(LATEST)"
+            )),
+            null
+        );
+        metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals(
+            "exp", "((apisix_nginx_http_current_connections.tag({tags -> tags.service_name = 1})).downsampling(LATEST))",
+            metricConvert.exp
+        );
+
+        // expPrefix is null
+        mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            "tag({tags -> tags.service_name = 2})",
+            null,
+            null,
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections.downsampling(LATEST)"
+            )),
+            null
+        );
+        metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals(
+            "exp", "(apisix_nginx_http_current_connections.downsampling(LATEST)).tag({tags -> tags.service_name = 2})",
+            metricConvert.exp
+        );
+
+        // expPrefix and expSuffix is null
+        mockMetricRuleConfig = new MockMetricRuleConfig(
+            "meter_apisix",
+            null,
+            null,
+            null,
+            Arrays.asList(new MockRule(
+                "sv_http_connections",
+                "apisix_nginx_http_current_connections"
+            )),
+            null
+        );
+        metricConvert = new MockMetricConvert(mockMetricRuleConfig, null);
+        Assert.assertEquals("metrics name", "meter_apisix_sv_http_connections", metricConvert.metricsName);
+        Assert.assertEquals(
+            "exp", "apisix_nginx_http_current_connections",
+            metricConvert.exp
+        );
+
+    }
+
+    static class MockMetricConvert extends MetricConvert {
+        private String metricsName;
+        private String filter;
+        private String exp;
+
+        public MockMetricConvert(final MetricRuleConfig rule, final MeterSystem service) {
+            super(rule, service);
+        }
+
+        @Override
+        Analyzer buildAnalyzer(final String metricsName,
+                               final String filter,
+                               final String exp,
+                               final MeterSystem service) {
+            this.metricsName = metricsName;
+            this.filter = filter;
+            this.exp = exp;
+            return null;
+        }
+
+    }
+
+    @Getter
+    @AllArgsConstructor
+    static class MockMetricRuleConfig implements MetricRuleConfig {
+        private String metricPrefix;
+        private String expSuffix;
+        private String expPrefix;
+        private String filter;
+        private List<MockRule> metricsRules;
+        private String initExp;
+
+    }
+
+    @Getter
+    @AllArgsConstructor
+    public static class MockRule implements MetricRuleConfig.RuleConfig {
+        private String name;
+        private String exp;
+    }
+}