[query] clean up logic of groupByNodes, implement nodes param in asPercent
#2816
Conversation
| func findFirstMetricExpression(seriesName string) string { | ||
| idxOfRightParen := strings.Index(seriesName, ")") | ||
| substring := seriesName[:idxOfRightParen] | ||
| idxOfLeftParen := strings.LastIndex(substring, "(") |
There was a problem hiding this comment.
Can you make findFirstMetricExpression potentially return an error if either strings.Index or strings.LastIndex return a -1?
Otherwise it will cause out of bounds panic, better to always optionally return an error rather than panic.
There was a problem hiding this comment.
Maybe return (string, bool) and if either of them return a -1 then the second bool param value returns false.
That way you can do:
func getParts(series *ts.Series) {
seriesName := series.Name()
if metricExpr, ok := findFirstMetricExpression(seriesName); ok {
seriesName = metricExpr
}
return strings.Split(seriesName, ".")
}There was a problem hiding this comment.
Done! Great idea.
| func getAggregationKey(series *ts.Series, nodes []int) string { | ||
| parts := getParts(series) | ||
|
|
||
| var keys []string |
There was a problem hiding this comment.
Can you pre-allocate this perhaps?
keys := make([]string, 0, len(nodes))| if n >= len(parts) || n < 0 { | ||
| return aggregate(ctx, series, fname) | ||
| if n < len(parts) { | ||
| keys = append(keys, parts[n]) |
There was a problem hiding this comment.
For graphite-web just an empty string is used if n is not within the range (see IndexError simply will "pass" which then tries to get the value from graphite tags or if not found returns empty string""):
https://github.com/graphite-project/graphite-web/blob/53a50b584016b889310eff346a2de01779613644/webapp/graphite/render/functions.py#L98-L115
To mimic same behavior instead of leaving it from the final key produced, I believe you want an else here:
if n < len(parts) {
keys = append(keys, parts[n])
} else {
keys = append(keys, "")
}That way you'll get "foo...bar" for two missing in the middle which better represents the true key since that part was missing and now won't collide with other values missing a value in a different key.
| if len(nodes) > 0 { | ||
| for _, s := range seriesList.Values { | ||
| key := getAggregationKey(s, nodes) | ||
| if key != "" { |
There was a problem hiding this comment.
graphite-web does not seem to perform this behavior, let's make this match graphite-web and always add the key to the grouping.
| case nil: | ||
| // if total is nil, the total is the sum of all the input series | ||
| toNormalize = input.Values | ||
| var err error = nil |
There was a problem hiding this comment.
Don't need to initialize to nil, just var err error will get default value.
* master: [read_index_segments] Always validate index segment checksums before reading/validating contents (#2835) [query] Return additional warnings in /query{,_range} endpoints (#2836) Add a check for seriesIter.Err (#2834) [tools] Add concurrent read_index_segments validation option (#2833) [query] Add non-ready namespaces to Clusters interface and use in /namespace/ready endpoint (#2828) [query] Tests for when function argument is an expression (#2809) [large-tiles] Additional metrics added (#2720) [query] Refactor groupByNodes and implement `nodes` arg in asPercent (#2816) [read_index_segments] Add log that outlines which segment being processed (#2825) [aggregator] Handle follower canLead() for not yet flushed windows (#2818)
What this PR does / why we need it:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: