Can't groupByNode(,,'highestCurrent')

[JaderDias]

groupByNode(,,'sum') works but 'highestCurrent' doesn't

[nnuss]

highestCurrent is being called but requires an additional 'n' argument which you are unable to pass through groupByNode.

[JaderDias]

@nnuss you're right, I forgot about this. I think I was expecting carbonapi to return highestCurrent(metrics, 1). Maybe we can solve this issue by making "1" the default value for highestCurrent, or we can simply mark this feature as "won't fix"

[nnuss]

In this specific case a default of 1 makes sense to me. When I test however I find more work is needed because highestCurrent is a filter (returns original series rather than a new reference/copy).

[nnuss]

Something like this could work but I'm unhappy enough with it (always walking all the map, append(results, r...) when r might now have >1, ... ) that I'm just putting it here for now. Thoughts?

(/cc @dgryski )

diff --git a/expr/expr.go b/expr/expr.go
index b3d1d0e..5fab9f0 100644
--- a/expr/expr.go
+++ b/expr/expr.go
@@ -1409,6 +1409,19 @@ func EvalExpr(e *expr, from, until int32, values map[MetricRequest][]*MetricData
                        }

                        r, _ := EvalExpr(nexpr, from, until, nvalues)
+
+                       // r could be a series in the values[MetricRequest][]*MetricData map.
+                       // We would not want to clobber it. Walk over the whole map.
+                       for _, vv := range values {
+                               for _, v := range vv {
+                                       if v == r[0] {
+                                               // replace with a copy
+                                               nr := *r[0]
+                                               r[0] = &nr
+                                               break
+                                       }
+                               }
+                       }
                        if r != nil {
                                r[0].Name = &k
                                results = append(results, r...)

[JaderDias]

I don't see why being a filter is a problem. I think it's a good thing when the expression doesn't modify its values.

[nnuss]

func EvalExpr( e *expr, from, until int32, values map[MetricRequest][]*MetricData ) ([]*MetricData, error)

values looks like:

values["metric1.*.total"] = [
&{"metric1.A.total", ...series data...},
&{"metric1.B.total", ...series data...},
&{"metric1.C.total", ...series data...},
&... more "metric.*.total" series ...
]

which are references to the metrics that we fetched from the stores and have created furing Eval.

If the callback creates a new series, such as 'sum' and others that are ultimately aggregateSeries(), then near the end of groupByName when we do r[0].Name = &k we don't care that we're changing the name because we created it.

However. If the callback is a filter and gives us a reference to something already within values ; which it does. Now if we blithely modify the name we can create confusion (weird legend name) or breakage (wrong number of nodes for substr etc).

Example: groupByNode(metric.*.total,2,highestCurrent) // I choose metric.C.total as the highest. Afterwards values looks like:

values["metric1.*.total"] = [
&{"metric1.A.total", ...series data...},
&{"metric1.B.total", ...series data...},
&{"total", ...series data...}, // At least it was a filter and the points don't change
&... more "metric.*.total" series ...
]

Now anybody following us [within this &target= expression I think] who operates on "metric1.*.total" is potentially in for a rude surprise. Expecially if they're doing any functions sensitive to the metric name.

[reyjrar]

Not sure I understand this, if you want the highest current in a groupByNodes(), isn't that just 'max' ? Maybe I'm missing something, but I see two possibilities:

Assuming something like:

sys.$datacenter.$server_role.$server_name.cpu.$cpu_number.{user,system,idle}

You could get the max CPU used by datacenter and server_role:

groupByNodes(sys.*.*.*.cpu.*.user, 'max', 1,2)

Or you could get the top 10 of those:

highestCurrent(groupByNodes(sys.*.*.*.cpu.*.user, 'max', 1,2),10)

Or if you wanted to do something weird, getting the average of the highest 10:

groupByNodes(highestCurrent(sys.*.*.*.cpu.*.user,10), 'avg', 1,2)

I'm missing the use case for highestCurrent() inside of a groupByNodes()

highestCurrent() is a filter, the position in groupByNodes() you're injecting highestCurrent() is expecting an aggregation function.

I think it's probably safe to close this issue.

[deniszh]

Hi Brad, Are you talking for some specific example or documentation? I think not all combinations of Graphite functions indeed makes sense...

[JaderDias]

Imagine you have 4 metrics

a.b 0,1
a.c 2,0
d.e 0,0
d.f 1,1

groupByNodes(*.*, 'max', 1)
groupByNodes(a.*, 'max', 1) 2,1 
groupByNodes(b.*, 'max', 1) 1,1

but what I needed 7 years ago when I opened this ticket was

groupByNodes(*.*, 'highestCurrent', 0)
a.b 0,1
d.f 1,1

[nnuss]

groupByNodes(*.*, 'highestCurrent', 0) would select

a.b 0,1   # but would be a new series "a"
d.f 1,1   # similarly "d"

because those have the highest value at the end of the series (last data point).

edit: intended first node which is 0

[JaderDias]

@nnuss you're right, fixed my previous comment

[JaderDias]

I just tested it again for the first time since 2017 and it seems that it works. I didn't check the exact numbers it provided, but it accepted the 'highestCurrent' as aggregation function and produced a reasonable graph.