From 5441f58f4cf2a3404a5035ffd594d54a32800c23 Mon Sep 17 00:00:00 2001 From: Wan Kai Date: Fri, 26 Jul 2024 18:49:33 +0800 Subject: [PATCH] MQE: check the metrics value before do binary operation to improve robustness (#12483) --- docs/en/changes/changes.md | 1 + .../skywalking/mqe/rt/operation/BinaryOp.java | 2 +- .../mqe/rt/operation/CompareOp.java | 2 +- .../skywalking/mqe/rt/operation/LROp.java | 65 ++++++++++++++++--- .../skywalking/mqe/rt/BinaryOpTest.java | 38 +++++++++++ .../apache/skywalking/mqe/rt/MockData.java | 7 ++ test/e2e-v2/script/env | 2 +- 7 files changed, 104 insertions(+), 13 deletions(-) diff --git a/docs/en/changes/changes.md b/docs/en/changes/changes.md index 12559cc48be4..7a6d526e804e 100644 --- a/docs/en/changes/changes.md +++ b/docs/en/changes/changes.md @@ -38,6 +38,7 @@ * Support BanyanDB internal stream query execution tracing. * Fix Elasticsearch, MySQL, RabbitMQ dashboards typos and missing expressions. * BanyanDB: Zipkin Module set service as Entity for improving the query performance. +* MQE: check the metrics value before do binary operation to improve robustness. #### UI diff --git a/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/BinaryOp.java b/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/BinaryOp.java index 00bfec134f01..0c84aaf98391 100644 --- a/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/BinaryOp.java +++ b/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/BinaryOp.java @@ -29,7 +29,7 @@ public static ExpressionResult doBinaryOp(ExpressionResult left, try { return LROp.doLROp(left, right, opType, BinaryOp::scalarBinaryOp); } catch (IllegalExpressionException e) { - throw new IllegalExpressionException("Unsupported binary operation."); + throw new IllegalExpressionException("Unsupported binary operation: " + e.getMessage()); } } diff --git a/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/CompareOp.java b/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/CompareOp.java index 499e7ca79e35..030f3174fde8 100644 --- a/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/CompareOp.java +++ b/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/CompareOp.java @@ -29,7 +29,7 @@ public static ExpressionResult doCompareOP(ExpressionResult left, try { return LROp.doLROp(left, right, opType, CompareOp::scalarCompareOp); } catch (IllegalExpressionException e) { - throw new IllegalExpressionException("Unsupported compare operation."); + throw new IllegalExpressionException("Unsupported compare operation: " + e.getMessage()); } } diff --git a/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/LROp.java b/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/LROp.java index c86ae6189391..4254b59898aa 100644 --- a/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/LROp.java +++ b/oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/LROp.java @@ -91,7 +91,17 @@ private static ExpressionResult single2SingleBinaryOp(ExpressionResult singleLef private static ExpressionResult single2SingleNoLabeled(ExpressionResult singleLeft, ExpressionResult singleRight, - int opType, LROp calculate) { + int opType, LROp calculate) throws IllegalExpressionException { + if (singleLeft.getResults().isEmpty() || + singleLeft.getResults().get(0).getValues().size() != 1) { + throw new IllegalExpressionException("Single to Single, left result is empty or has more than one value."); + } + + if (singleRight.getResults().isEmpty() || + singleRight.getResults().get(0).getValues().size() != 1) { + throw new IllegalExpressionException("Single to Single, right result is empty or has more than one value."); + } + ExpressionResult result = new ExpressionResult(); MQEValue mqeValue = new MQEValue(); MQEValues mqeValues = new MQEValues(); @@ -119,12 +129,18 @@ private static ExpressionResult single2SingleLabeled(ExpressionResult singleLeft labelMapR.put(new HashSet<>(mqeValuesR.getMetric().getLabels()), mqeValuesR.getValues()); }); for (MQEValues mqeValuesL : singleLeft.getResults()) { + if (mqeValuesL.getValues().size() != 1) { + throw new IllegalExpressionException("Single Labeled to Single Labeled, left labeled result is empty or has more than one value."); + } //reserve left metric info MQEValue valueL = mqeValuesL.getValues().get(0); List mqeValuesR = labelMapR.get(new HashSet<>(mqeValuesL.getMetric().getLabels())); if (mqeValuesR == null) { valueL.setEmptyValue(true); } else { + if (mqeValuesR.size() != 1) { + throw new IllegalExpressionException("Single Labeled to Single Labeled, right labeled result has more than one value."); + } MQEValue valueR = mqeValuesR.get(0); if (valueL.isEmptyValue() || valueR.isEmptyValue()) { valueL.setEmptyValue(true); @@ -141,7 +157,11 @@ private static ExpressionResult single2SingleLabeled(ExpressionResult singleLeft //series or list or labeled single value with scalar private static ExpressionResult many2OneBinaryOp(ExpressionResult manyResult, ExpressionResult singleResult, - int opType, LROp calculate) { + int opType, LROp calculate) throws IllegalExpressionException { + if (singleResult.getResults().isEmpty() || + singleResult.getResults().get(0).getValues().size() != 1) { + throw new IllegalExpressionException("Many to One, single result is empty or has more than one value."); + } manyResult.getResults().forEach(mqeValues -> { mqeValues.getValues().forEach(mqeValue -> { if (!mqeValue.isEmptyValue()) { @@ -161,7 +181,11 @@ private static ExpressionResult many2OneBinaryOp(ExpressionResult manyResult, //scalar with series or list or labeled single value private static ExpressionResult one2ManyBinaryOp(ExpressionResult singleResult, ExpressionResult manyResult, - int opType, LROp calculate) { + int opType, LROp calculate) throws IllegalExpressionException { + if (singleResult.getResults().isEmpty() || + singleResult.getResults().get(0).getValues().size() != 1) { + throw new IllegalExpressionException("One to Many, single result is empty or has more than one value."); + } manyResult.getResults().forEach(mqeValues -> { mqeValues.getValues().forEach(mqeValue -> { if (!mqeValue.isEmptyValue()) { @@ -182,9 +206,15 @@ private static ExpressionResult one2ManyBinaryOp(ExpressionResult singleResult, private static ExpressionResult seriesNoLabeled(ExpressionResult seriesLeft, ExpressionResult seriesRight, - int opType, LROp calculate) { + int opType, LROp calculate) throws IllegalExpressionException { + if (seriesLeft.getResults().isEmpty() || seriesRight.getResults().isEmpty()) { + throw new IllegalExpressionException("Series No Labeled, left or right result is empty."); + } MQEValues mqeValuesL = seriesLeft.getResults().get(0); MQEValues mqeValuesR = seriesRight.getResults().get(0); + if (mqeValuesL.getValues().size() != mqeValuesR.getValues().size()) { + throw new IllegalExpressionException("Series No Labeled, left and right series value size not equal."); + } for (int i = 0; i < mqeValuesL.getValues().size(); i++) { //clean metric info MQEValue valueL = mqeValuesL.getValues().get(i); @@ -203,9 +233,15 @@ private static ExpressionResult seriesNoLabeled(ExpressionResult seriesLeft, private static ExpressionResult seriesLabeledWithNoLabeled(ExpressionResult seriesLeft, ExpressionResult seriesRight, - int opType, LROp calculate) { + int opType, LROp calculate) throws IllegalExpressionException { + if (seriesRight.getResults().isEmpty()) { + throw new IllegalExpressionException("Series Labeled with No Labeled, no labeled result is empty."); + } MQEValues mqeValuesR = seriesRight.getResults().get(0); - seriesLeft.getResults().forEach(mqeValuesL -> { + for (MQEValues mqeValuesL : seriesLeft.getResults()) { + if (mqeValuesL.getValues().size() != mqeValuesR.getValues().size()) { + throw new IllegalExpressionException("Series Labeled with No Labeled, left and right series value size not equal."); + } for (int i = 0; i < mqeValuesL.getValues().size(); i++) { //reserve left metric info MQEValue valueL = mqeValuesL.getValues().get(i); @@ -217,16 +253,22 @@ private static ExpressionResult seriesLabeledWithNoLabeled(ExpressionResult seri double newValue = calculate.apply(valueL.getDoubleValue(), valueR.getDoubleValue(), opType); mqeValuesL.getValues().get(i).setDoubleValue(newValue); } - }); + } return seriesLeft; } private static ExpressionResult seriesNoLabeledWithLabeled(ExpressionResult seriesLeft, ExpressionResult seriesRight, - int opType, LROp calculate) { + int opType, LROp calculate) throws IllegalExpressionException { + if (seriesLeft.getResults().isEmpty()) { + throw new IllegalExpressionException("Series No Labeled with Labeled, no labeled result is empty."); + } MQEValues mqeValuesL = seriesLeft.getResults().get(0); - seriesRight.getResults().forEach(mqeValuesR -> { + for (MQEValues mqeValuesR : seriesRight.getResults()) { + if (mqeValuesL.getValues().size() != mqeValuesR.getValues().size()) { + throw new IllegalExpressionException("Series No Labeled with Labeled, left and right series value size not equal."); + } for (int i = 0; i < mqeValuesL.getValues().size(); i++) { //reserve left metric info MQEValue valueL = mqeValuesL.getValues().get(i); @@ -238,7 +280,7 @@ private static ExpressionResult seriesNoLabeledWithLabeled(ExpressionResult seri double newValue = calculate.apply(valueL.getDoubleValue(), valueR.getDoubleValue(), opType); mqeValuesR.getValues().get(i).setDoubleValue(newValue); } - }); + } return seriesRight; } @@ -258,6 +300,9 @@ private static ExpressionResult seriesLabeledWithLabeled(ExpressionResult series if (mqeValuesR == null) { valueL.setEmptyValue(true); } else { + if (mqeValuesR.size() != mqeValuesL.getValues().size()) { + throw new IllegalExpressionException("Series Labeled with Labeled, left and right series value size not equal."); + } MQEValue valueR = mqeValuesR.get(i); if (valueL.isEmptyValue() || valueR.isEmptyValue()) { valueL.setEmptyValue(true); diff --git a/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/BinaryOpTest.java b/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/BinaryOpTest.java index 74b458c106ba..ea5e3d56f349 100644 --- a/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/BinaryOpTest.java +++ b/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/BinaryOpTest.java @@ -26,6 +26,7 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; public class BinaryOpTest { private final MockData mockData = new MockData(); @@ -33,6 +34,10 @@ public class BinaryOpTest { //DIV/MUL/MOD/SUB... are the same logic and tested in here, the others only test ADD is enough. @Test public void seriesNoLabeledTest() throws Exception { + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, false), + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, false), MQEParser.ADD + )); ExpressionResult add = BinaryOp.doBinaryOp( mockData.newSeriesNoLabeledResult(), mockData.newSeriesNoLabeledResult(), MQEParser.ADD); assertEquals(ExpressionResultType.TIME_SERIES_VALUES, add.getType()); @@ -76,6 +81,10 @@ public void seriesNoLabeledTest() throws Exception { @Test public void seriesLabeledTest() throws Exception { + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newSeriesLabeledResult(), + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, false), MQEParser.ADD + )); //seriesLabeled + seriesNoLabeled ExpressionResult add = BinaryOp.doBinaryOp(mockData.newSeriesLabeledResult(), mockData.newSeriesNoLabeledResult(), MQEParser.ADD); assertEquals(ExpressionResultType.TIME_SERIES_VALUES, add.getType()); @@ -133,6 +142,18 @@ public void seriesLabeledTest() throws Exception { @Test public void many2OneTest() throws Exception { + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, false), + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, false), MQEParser.ADD + )); + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, true), + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, false), MQEParser.ADD + )); + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, false), + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, true), MQEParser.ADD + )); //sort_list + single ExpressionResult add = BinaryOp.doBinaryOp( mockData.newListResult(), mockData.newSingleResult(1000), MQEParser.ADD); @@ -171,6 +192,14 @@ public void many2OneTest() throws Exception { @Test public void one2ManyTest() throws Exception { + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, false), + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, false), MQEParser.ADD + )); + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, false), + mockData.newEmptyResult(ExpressionResultType.TIME_SERIES_VALUES, true), MQEParser.ADD + )); // single - sort_list ExpressionResult sub = BinaryOp.doBinaryOp( mockData.newSingleResult(1000), mockData.newListResult(), MQEParser.SUB); @@ -209,6 +238,15 @@ public void one2ManyTest() throws Exception { @Test public void single2SingleTest() throws IllegalExpressionException { + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, false), + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, false), MQEParser.ADD + )); + assertThrows(IllegalExpressionException.class, () -> BinaryOp.doBinaryOp( + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, false), + mockData.newEmptyResult(ExpressionResultType.SINGLE_VALUE, true), MQEParser.ADD + )); + //noLabeled + noLabeled ExpressionResult add = BinaryOp.doBinaryOp( mockData.newSingleResult(100), mockData.newSingleResult(200), MQEParser.ADD); diff --git a/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/MockData.java b/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/MockData.java index 2a53df8f0dca..ff25c1e9974c 100644 --- a/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/MockData.java +++ b/oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/MockData.java @@ -185,6 +185,13 @@ public ExpressionResult newSingleLabeledResult(double id1001, double id1002) { return result; } + public ExpressionResult newEmptyResult(ExpressionResultType type, boolean labeled) { + ExpressionResult result = new ExpressionResult(); + result.setType(type); + result.setLabeledResult(labeled); + return result; + } + public MQEValue newMQEValue(String id, double value) { MQEValue mqeValue = new MQEValue(); mqeValue.setId(id); diff --git a/test/e2e-v2/script/env b/test/e2e-v2/script/env index 3d61b338bd18..b495437dde9e 100644 --- a/test/e2e-v2/script/env +++ b/test/e2e-v2/script/env @@ -23,7 +23,7 @@ SW_AGENT_CLIENT_JS_COMMIT=af0565a67d382b683c1dbd94c379b7080db61449 SW_AGENT_CLIENT_JS_TEST_COMMIT=4f1eb1dcdbde3ec4a38534bf01dded4ab5d2f016 SW_KUBERNETES_COMMIT_SHA=1335f15bf821a40a7cd71448fa805f0be265afcc SW_ROVER_COMMIT=6bbd39aa701984482330d9dfb4dbaaff0527d55c -SW_BANYANDB_COMMIT=285db188e633c6b95b8c5c354e043db79658c147 +SW_BANYANDB_COMMIT=6f938ccf812e2183b4bd891fb90b2124aa65c170 SW_AGENT_PHP_COMMIT=3192c553002707d344bd6774cfab5bc61f67a1d3 SW_CTL_COMMIT=d5f3597733aa5217373986d776a3ee5ee8b3c468