Skip to content

Commit

Permalink
MQE: check the metrics value before do binary operation to improve ro…
Browse files Browse the repository at this point in the history 
…bustness (#12483)
  • Loading branch information
@wankai123
wankai123 committed Jul 26, 2024
1 parent 63b3020 commit 5441f58
Show file tree
Hide file tree
Showing 7 changed files with 104 additions and 13 deletions.
1 change: 1 addition & 0 deletions docs/en/changes/changes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/BinaryOp.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

Expand Down
2 changes: 1 addition & 1 deletion oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/CompareOp.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

Expand Down
65 changes: 55 additions & 10 deletions oap-server/mqe-rt/src/main/java/org/apache/skywalking/mqe/rt/operation/LROp.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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<MQEValue> 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);
Expand All @@ -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()) {
Expand All @@ -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()) {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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;
}
Expand All @@ -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);
Expand Down
38 changes: 38 additions & 0 deletions oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/BinaryOpTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,18 @@
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();

//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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 7 additions & 0 deletions oap-server/mqe-rt/src/test/java/org/apache/skywalking/mqe/rt/MockData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion test/e2e-v2/script/env
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 5441f58

Please sign in to comment.