From c52aa02341e1dd7efafddbfceb4434aa0006e6d8 Mon Sep 17 00:00:00 2001
From: wankai123 <wankai123@foxmail.com>
Date: Mon, 25 Jan 2021 12:56:06 +0800
Subject: [PATCH] Fix bug that the same sample name in one MAL expression
 caused `IllegalArgumentException` in `Analyzer.analyse`. (#6250)

---
 CHANGES.md                                         |  1 +
 .../oap/meter/analyzer/dsl/Expression.java         |  6 +++++-
 .../oap/meter/analyzer/dsl/ArithmeticTest.java     | 14 ++++++++++++++
 .../meter/analyzer/dsl/ExpressionParsingTest.java  | 13 +++++++++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/CHANGES.md b/CHANGES.md
index 2cdc4f345..2b642270f 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -75,6 +75,7 @@ Release Notes.
 * Fix bug in `parseInternalErrorCode` where some error codes are never reached.
 * OAL supports multiple values when as numeric
 * Add node information from the Openensus proto to the labels of the samples, to support the identification of the source of the Metric data.
+* Fix bug that the same sample name in one MAL expression caused `IllegalArgumentException` in `Analyzer.analyse`.
 
 #### UI
 * Fix un-removed tags in trace query.
diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/dsl/Expression.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/dsl/Expression.java
index 7cf84f66b..7a3b2547b 100644
--- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/dsl/Expression.java
+++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/dsl/Expression.java
@@ -95,7 +95,11 @@ public class Expression {
         expression.setDelegate(new GroovyObjectSupport() {
 
             public SampleFamily propertyMissing(String metricName) {
-                ExpressionParsingContext.get().ifPresent(ctx -> ctx.samples.add(metricName));
+                ExpressionParsingContext.get().ifPresent(ctx -> {
+                    if (!ctx.samples.contains(metricName)) {
+                        ctx.samples.add(metricName);
+                    }
+                });
                 ImmutableMap<String, SampleFamily> sampleFamilies = propertyRepository.get();
                 if (sampleFamilies == null) {
                     return SampleFamily.EMPTY;
diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ArithmeticTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ArithmeticTest.java
index 3ea9fee59..e755cf4e2 100644
--- a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ArithmeticTest.java
+++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ArithmeticTest.java
@@ -238,6 +238,20 @@ public class ArithmeticTest {
                 ).build()),
                 false,
             },
+            {
+                "sameSampleFamily-minus-sameSampleFamily",
+                of("http_success_request", SampleFamilyBuilder.newBuilder(
+                    Sample.builder().labels(of("idc", "t1" , "service", "service1")).value(100).build(),
+                    Sample.builder().labels(of("idc", "t2" , "service", "service1")).value(30).build(),
+                    Sample.builder().labels(of("idc", "t3" , "service", "service1")).value(40).build(),
+                    Sample.builder().labels(of("region", "us" , "service", "service1")).value(80).build()
+                ).build()),
+                "http_success_request.sum(['service']) - http_success_request.sum(['service'])",
+                Result.success(SampleFamilyBuilder.newBuilder(
+                    Sample.builder().labels(of("service", "service1")).value(0).build()
+                ).build()),
+                false,
+                },
             {
                 "empty-multiple-empty",
                 of("http_success_request", SampleFamily.EMPTY,
diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ExpressionParsingTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ExpressionParsingTest.java
index cca1801e1..4bb74ba2e 100644
--- a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ExpressionParsingTest.java
+++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/ExpressionParsingTest.java
@@ -18,6 +18,7 @@
 
 package org.apache.skywalking.oap.meter.analyzer.dsl;
 
+import com.google.common.collect.Lists;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.skywalking.oap.server.core.analysis.meter.ScopeType;
 import org.junit.Test;
@@ -74,6 +75,18 @@ public class ExpressionParsingTest {
                                         .percentiles(new int[]{50, 99}).build(),
                 false,
             },
+            {
+                "sameSamples",
+                "(node_cpu_seconds_total.sum(['node_identifier_host_name']) - node_cpu_seconds_total.tagEqual('mode', 'idle').sum(['node_identifier_host_name'])).service(['node_identifier_host_name']) ",
+                ExpressionParsingContext.builder()
+                                        .samples(Collections.singletonList("node_cpu_seconds_total"))
+                                        .scopeType(ScopeType.SERVICE)
+                                        .scopeLabels(Collections.singletonList("node_identifier_host_name"))
+                                        .aggregationLabels(Lists.newArrayList("node_identifier_host_name" , "node_identifier_host_name"))
+                                        .downsampling(DownsamplingType.AVG)
+                                        .isHistogram(false).build(),
+                false,
+                },
         });
     }