[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #6427: Add `sum` function in meter system

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #6427:
URL: https://github.com/apache/skywalking/pull/6427#discussion_r581128829



##########
File path: oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/dsl/SampleFamily.java
##########
@@ -175,7 +176,10 @@ public SampleFamily min(List<String> by) {
     }
 
     public SampleFamily avg(List<String> by) {
-        ExpressionParsingContext.get().ifPresent(ctx -> ctx.aggregationLabels.addAll(by));
+        ExpressionParsingContext.get().ifPresent(ctx -> {
+            ctx.aggregationLabels.addAll(by);
+            ctx.downsampling = DownsamplingType.AVG;

Review comment:
       > AVG seems default in `ExpressionParsingContext#create`. Is this for making codes more clear or functional need?
   
   `avg` being default had no problem before since it was only one function, now since I added another one, if the expression contains multiple function, and the `avg` being last one, it may be wrong.  For example, say the expression is `log_count.sum(by: ['service', 'instance']).avg(['level'])`, (this expression may make no sense, just for demonstration), the final metrics should be aggregated by `avg` instead of `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.

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