You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "weixiang1862 (via GitHub)" <gi...@apache.org> on 2023/07/03 10:14:15 UTC

[GitHub] [skywalking] weixiang1862 opened a new pull request, #11036: Support reduce aggregate function in MQE.(#11031)

weixiang1862 opened a new pull request, #11036:
URL: https://github.com/apache/skywalking/pull/11036

   <!--
       ⚠️ Please make sure to read this template first, pull requests that don't accord with this template
       maybe closed without notice.
       Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
       Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
   -->
   
   <!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
   ### Fix <bug description or the bug issue number or bug issue link>
   - [ ] Add a unit test to verify that the fix works.
   - [ ] Explain briefly why the bug exists and how to fix it.
        ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
   
   <!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
   ### Improve the performance of <class or module or ...>
   - [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
   - [ ] The benchmark result.
   ```text
   <Paste the benchmark results here>
   ```
   - [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
        ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
   
   
   ### reduce function
   - [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
   - [x] Update the documentation to include this new feature.
   - [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
   - [x] If it's UI related, attach the screenshots below.
   ![55567bee522b0cdf2e797f546670b27](https://github.com/apache/skywalking/assets/16346644/a87a789d-bfa4-4cf7-b0e6-e288f6c234f9)
   
   
   - [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #11031 .
   - [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1617872775

   Reduce seems a strange name, Could you explain the logic?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on a diff in pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#discussion_r1252443286


##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/operation/AggregateLabelOp.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.query.graphql.mqe.rt.operation;
+
+import java.util.Collections;
+import java.util.List;
+import org.apache.skywalking.mqe.rt.grammar.MQEParser;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.exception.IllegalExpressionException;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.AvgAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.MaxAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.MinAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.SumAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.AggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.AggregateLabelFuncFactory;
+import org.apache.skywalking.oap.query.graphql.type.mql.ExpressionResult;
+import org.apache.skywalking.oap.query.graphql.type.mql.MQEValue;
+import org.apache.skywalking.oap.query.graphql.type.mql.MQEValues;
+
+public class AggregateLabelOp {
+
+    public static ExpressionResult doAggregateLabelOp(ExpressionResult result,
+                                                      int opType) throws IllegalExpressionException {
+        switch (opType) {
+            case MQEParser.AVG:
+                return aggregateLabeledValueResult(result, AvgAggregateLabelFunc::new);
+            case MQEParser.SUM:
+                return aggregateLabeledValueResult(result, SumAggregateLabelFunc::new);
+            case MQEParser.MAX:
+                return aggregateLabeledValueResult(result, MaxAggregateLabelFunc::new);
+            case MQEParser.MIN:
+                return aggregateLabeledValueResult(result, MinAggregateLabelFunc::new);
+            default:
+                throw new IllegalExpressionException("Unsupported aggregateLabel function.");
+        }
+    }
+
+    private static ExpressionResult aggregateLabeledValueResult(ExpressionResult expResult,
+                                                                AggregateLabelFuncFactory factory) {
+        List<MQEValues> results = expResult.getResults();
+
+        List<MQEValue> combineTo = results.get(0).getValues();
+        for (int i = 0; i < combineTo.size(); i++) {
+            AggregateLabelFunc aggregateLabelFunc = factory.getAggregateLabelFunc();
+            for (MQEValues result : results) {
+                aggregateLabelFunc.combine(result.getValues().get(i).getDoubleValue());
+            }
+            combineTo.get(i).setDoubleValue(aggregateLabelFunc.getResult());
+        }
+
+        MQEValues mqeValues = new MQEValues();
+        mqeValues.setValues(combineTo);
+        expResult.setResults(Collections.singletonList(mqeValues));

Review Comment:
   ```suggestion
           expResult.setResults(Collections.singletonList(mqeValues));
           expResult..setLabeledResult(false);
   ```



-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on a diff in pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#discussion_r1252494499


##########
docs/en/api/metrics-query-expression.md:
##########
@@ -178,3 +178,28 @@ relabel(service_percentile{label='0,1,2,3,4'}, label='P50,P75,P90,P95,P99')
 
 ### Result Type
 Follow the input expression.
+
+## AggregateLabel Operation
+AggregateLabel Operation takes an expression and performs aggregate calculation on its labeled value results. It aggregates a group of TIME_SERIES_VALUES into a single TIME_SERIES_VALUES.

Review Comment:
   The original data format should be labeled value metric? Please pick the term of type SkyWalking defined



-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1617966072

   I think that makes more sense and clear.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1617815496

   @wu-sheng @wankai123  It is not the final code, I want to confirm the following questions:
   1. whether `reduce` is a good name?
   2. `reduce` should be defined as function or aggregate function?
   3. whether reduce avg agorithm should filter null values?
   4. I use `(COMMA parameter)*` define aggregate function param list, It means aggregation function can have 0-N params, I'm worried it may effect other agg function，should I define it like 'function0' 'function1'?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620008180

   OK. Make sense. One tip, the word `label` seems not precious. Could you support `labels` and put that in the docs?
   I know it is a breaking change, but a label with multiple values seem not good for the longer term. @wankai123 


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on a diff in pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#discussion_r1252443286


##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/operation/AggregateLabelOp.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.query.graphql.mqe.rt.operation;
+
+import java.util.Collections;
+import java.util.List;
+import org.apache.skywalking.mqe.rt.grammar.MQEParser;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.exception.IllegalExpressionException;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.AvgAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.MaxAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.MinAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.SumAggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.AggregateLabelFunc;
+import org.apache.skywalking.oap.query.graphql.mqe.rt.operation.aggregatelabel.AggregateLabelFuncFactory;
+import org.apache.skywalking.oap.query.graphql.type.mql.ExpressionResult;
+import org.apache.skywalking.oap.query.graphql.type.mql.MQEValue;
+import org.apache.skywalking.oap.query.graphql.type.mql.MQEValues;
+
+public class AggregateLabelOp {
+
+    public static ExpressionResult doAggregateLabelOp(ExpressionResult result,
+                                                      int opType) throws IllegalExpressionException {
+        switch (opType) {
+            case MQEParser.AVG:
+                return aggregateLabeledValueResult(result, AvgAggregateLabelFunc::new);
+            case MQEParser.SUM:
+                return aggregateLabeledValueResult(result, SumAggregateLabelFunc::new);
+            case MQEParser.MAX:
+                return aggregateLabeledValueResult(result, MaxAggregateLabelFunc::new);
+            case MQEParser.MIN:
+                return aggregateLabeledValueResult(result, MinAggregateLabelFunc::new);
+            default:
+                throw new IllegalExpressionException("Unsupported aggregateLabel function.");
+        }
+    }
+
+    private static ExpressionResult aggregateLabeledValueResult(ExpressionResult expResult,
+                                                                AggregateLabelFuncFactory factory) {
+        List<MQEValues> results = expResult.getResults();
+
+        List<MQEValue> combineTo = results.get(0).getValues();
+        for (int i = 0; i < combineTo.size(); i++) {
+            AggregateLabelFunc aggregateLabelFunc = factory.getAggregateLabelFunc();
+            for (MQEValues result : results) {
+                aggregateLabelFunc.combine(result.getValues().get(i).getDoubleValue());
+            }
+            combineTo.get(i).setDoubleValue(aggregateLabelFunc.getResult());
+        }
+
+        MQEValues mqeValues = new MQEValues();
+        mqeValues.setValues(combineTo);
+        expResult.setResults(Collections.singletonList(mqeValues));

Review Comment:
   ```suggestion
           expResult.setResults(Collections.singletonList(mqeValues));
           expResult.setLabeledResult(false);
   ```



-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620990751

   > One thing about the function name, from your current logic, it seems all labels are going to be merged, right?
   
   Yes, all labels will be merged.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1621008155

   > > > One thing about the function name, from your current logic, it seems all labels are going to be merged, right?
   > > 
   > > 
   > > Yes, all labels will be merged.
   > 
   > Is aggregate_labels better?
   aggregate_labels beter than aggregate_label, I will change name to this, thanks.
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1618142521

   > 1. whether `reduce` is a good name?
   
   aggregate_label or something? I think it's only for the `labeled_value` series.
   > 2. `reduce` should be defined as function or aggregate function?
   
   see https://github.com/apache/skywalking/pull/11036#discussion_r1250789220
   > 3. whether reduce avg agorithm should filter null values?
   
   I think should reserve NULL if the result is NULL
   > 4. I use `(COMMA parameter)*` define aggregate function param list, It means aggregation function can have 0-N params, I'm worried it may effect other agg function，should I define it like 'function0' 'function1'?
   
   please refer to https://github.com/apache/skywalking/pull/11036#discussion_r1250789220
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng merged pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #11036:
URL: https://github.com/apache/skywalking/pull/11036


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620874861

   I hava completed the code adjustment, add introduce docs and tests about aggregate_label function, please help me review.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1621402296

   I have changed function name to `aggregate_labels` and add an e2e case for this function.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on a diff in pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on code in PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#discussion_r1250942980


##########
oap-server/server-query-plugin/mqe-grammar/src/main/antlr4/org/apache/skywalking/mqe/rt/grammar/MQELexer.g4:
##########
@@ -72,6 +73,7 @@ DES options { caseInsensitive=true; }: 'des';
 
 // Literals
 INTEGER: Digit+;
+STRING_PARAM: '\'' NameLetter+ '\'';

Review Comment:
   Ok，thanks. I will fix these.



-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619923030

   > mongodb_cluster_connection{label="1,2,3,4"}
   
   This style works for me. Question would be, are labels always predicted? If not, do we support all or some sub expression?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1617937410

   I think we should use aggregate with function name, with a parameter to match labels, star(*) or a list of known labels.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619971859

   > This style works for me. Question would be, are labels always predicted? If not, do we support all or some sub expression?
   
   `mongodb_cluster_connection{label="1,2,3,4"}` is a metric_expression already supported by MQE and returns time series labeled value.  So the labels should be predicted, and the  new `aggregate_label ` func just needs to aggregate the labels in the result of `mongodb_cluster_connection{label="1,2,3,4"}`.
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on a diff in pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#discussion_r1250789220


##########
oap-server/server-query-plugin/mqe-grammar/src/main/antlr4/org/apache/skywalking/mqe/rt/grammar/MQEParser.g4:
##########
@@ -27,7 +27,7 @@ expression
     | expression mulDivMod expression  # mulDivModOp
     | expression addSub expression     # addSubOp
     | expression compare expression    # compareOp
-    | aggregation L_PAREN expression R_PAREN # aggregationOp
+    | aggregation L_PAREN expression (COMMA parameter)* R_PAREN # aggregationOp

Review Comment:
   It's better to create a new expression like `top_n` or `relabels`. An expression should have determined parameters to avoid unexpected parsing.



-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on a diff in pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#discussion_r1250810553


##########
oap-server/server-query-plugin/mqe-grammar/src/main/antlr4/org/apache/skywalking/mqe/rt/grammar/MQELexer.g4:
##########
@@ -72,6 +73,7 @@ DES options { caseInsensitive=true; }: 'des';
 
 // Literals
 INTEGER: Digit+;
+STRING_PARAM: '\'' NameLetter+ '\'';

Review Comment:
   if the params are `sum/max/avg..`, should be better to define them rather than use `String`. Can refer to the `order` https://github.com/apache/skywalking/blob/de3a49ef7b5631f4930d33c0f33b3ce07f461af9/oap-server/server-query-plugin/mqe-grammar/src/main/antlr4/org/apache/skywalking/mqe/rt/grammar/MQEParser.g4#L70



-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620891652

   
   
   
   > Could you add an `e2e case` to verify the new expression? You can use `service_percentile` metric and refer here:
   > 
   > https://github.com/apache/skywalking/blob/8aab7d7ac7386e2a1b0dc8af995ceb1c6715d2ff/test/e2e-v2/cases/mqe/mqe-cases.yaml#L48
   
   Ok, I will add a e2e case.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619862906

   What do you mean? D you mean it is supported or what?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620206284

   > OK. Make sense. One tip, the word `label` seems not precious. Could you support `labels` and put that in the docs? I know it is a breaking change, but a label with multiple values seem not good for the longer term. @wankai123
   
   I think it's related to Skywalking's label mechanism, we could discuss it later.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1617908157

   > Reduce seems a strange name, Could you explain the logic?
   
   reduce merge several time serial data into one，in dashboard it looks like several line merged into one line. Essentially, it aggregate labelvalue to singlevalue.
   
   reduce name is reference from java stream reduce. I feel it strange too, so I ask you for help.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620890360

   Could you add an `e2e case` to verify the new expression? You can use `service _percentile` metric and refer here: https://github.com/apache/skywalking/blob/8aab7d7ac7386e2a1b0dc8af995ceb1c6715d2ff/test/e2e-v2/cases/mqe/mqe-cases.yaml#L48


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619875370

   So, no need for a new PR to support?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1621554204

   I rerun, it works. 


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619852049

   @wu-sheng I think aggregate function does not need a label param to match labels, because metric name expression already have this ability implemented by @wankai123 , the following two expression is equivalent：
   ```
   aggregate_label(mongodb_cluster_connection{label="1,2,3,4"}, SUM)
   
   aggregate_label(mongodb_cluster_connection, {label="1,2,3,4"}, SUM)
   ```


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619887837

   > So, no need for a new PR to support?
   
   Sorry, I didn't explain it clearly.
   
   aggregate_label function is need, and I will perfect this pr according to your requirements we talked yesterday.
   
   But this function does not need a label param to match label because it is already supported by metric_expression.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wankai123 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619878075

   > @wu-sheng I think aggregate function does not need a label param to match labels, because metric name expression already have this ability implemented by @wankai123 , the following two expression is equivalent：
   > 
   > ```
   > aggregate_label(mongodb_cluster_connection{label="1,2,3,4"}, SUM)
   > 
   > aggregate_label(mongodb_cluster_connection, {label="1,2,3,4"}, SUM)
   > ```
   
   yes, the `aggregate_label` should base on the result of the `labeled_value expression`. Just .ike `avg/min` or other functions.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620996120

   > > One thing about the function name, from your current logic, it seems all labels are going to be merged, right?
   > 
   > 
   > 
   > Yes, all labels will be merged.
   
   Is aggregate_labels better?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1619872678

   > What do you mean? D you mean it is supported or what?
   
   I means `label match` is supported, if we aggregate all label values, we can define MQE like this:
   ```
   aggregate_label(mongodb_cluster_connection, SUM)
   ```
   If we just want aggregate part of labels, we can define MQE like this:
   ```
   aggregate_label(mongodb_cluster_connection{label='1,2,3,4'}, SUM)
   ```


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1617963145

   > I think we should use aggregate as function name, with a parameter to match labels, star(*) or a list of known labels.
   > 
   > And, how would you process the number? avg, max, min? I think it should be a parameter too.
   
   Like this?
   ```
   aggregate(metric_name, by_label, sum|avg|max)
   ```


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1617981806

   > I think that makes more sense and clear.
   
   Ok, thanks. I will adjust code like this.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] weixiang1862 commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "weixiang1862 (via GitHub)" <gi...@apache.org>.
weixiang1862 commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1621505860

   Anything I can do? I found there are 4 failings.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #11036: Support reduce aggregate function in MQE.(#11031)

Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #11036:
URL: https://github.com/apache/skywalking/pull/11036#issuecomment-1620527106

   Sync discussion conclusion, the default label key would be change to _(under line).


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org