Sum values in unwrapped rate aggregation instead of treating them as counter

[chaudum]

What this PR does / why we need it:

This PR implements the first part of the RFC described in #6351

It reverts rate() to its previous implementation prior to #5013 That means it calculates the per-second rate from the sum of all extracted values.

Which issue(s) this PR fixes:

#6351

Checklist

• Documentation added
• Tests updated
• Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
-        distributor	-0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
-              logql	-0.2%
+               loki	0%

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

-           ingester	-0.1%
-        distributor	-0.3%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
-              logql	-0.2%
+               loki	0%

[chaudum]

@liguozhong Wanted to give you a heads up because it probably affects you.

[liguozhong]

@liguozhong Wanted to give you a heads up because it probably affects you.

Thanks , I got it

[DylanGuedes]

Few nits.

Btw, do you mind renaming the PR title to something like what you have in the CHANGELOG? In the changelog it is described as "Sum values in unwrapped rate aggregation instead of treating them as counter".

[chaudum]

Few nits.

Btw, do you mind renaming the PR title to something like what you have in the CHANGELOG? In the changelog it is described as "Sum values in unwrapped rate aggregation instead of treating them as counter".

Let me make two separate pull requests. Then the PR title also matches the changes.

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
-              logql	-0.2%
+               loki	0%

[DylanGuedes]

lgtm! (the jsonnet failure isn't related to your change)

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
-        distributor	-0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
-              logql	-0.2%
+               loki	0%

[trevorwhitney]

so, I don't fully understand this PR. for one, why are all the values we assert against in the tests changing? does this actually change computation behavior? was the previous implementation incorrect?

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
-              logql	-0.2%
+               loki	0%

[ssncferreira]

I'm having some difficulty understanding these tests 😕
What does the SUM(n=47, 61, 1) mean? I assume this is the result of the newSeries method on the data property, but don't understand how these values were calculated 🤔

[chaudum]

Regarding the SUM(n=47, 61, 1): it just means the sum of the values from item 47 to item 61 where the value is a constant of 1, like https://www.wolframalpha.com/input?i2d=true&i=Sum%5B1%2C%7Bn%2C47%2C61%7D%5D

Regarding, why 47, 61, and 1, we have to look at the output of the newSeries() function: It creates a stream {app="foo"} with 300 samples starting at timestamp 46e9 (46s) and ending at timestamp 345e9 (345s). The sample value is a constant 1.

So the query now looks at the time range from ts=30s to ts=60s, where the lower bound is not included. There are 15 items (item 47 to item 61) from the generated series that matches.

Hope this helps.

[ssncferreira]

Thank you! 🙏 This helps a lot.
Would it be possible to add this information in the comments to make the tests easier to understand? e.g.

// create a stream {app="foo"} with 300 samples starting at 46s and ending at 345s with a constant value of 1
[][]logproto.Series{
  {newSeries(testSize, offset(46, constantValue(1)), `{app="foo"}`)},
},
// query between the time range from ts=30s and ts=60s where the lower bound is not included
[]SelectSampleParams{
  {&logproto.SampleQueryRequest{Start: time.Unix(30, 0), End: time.Unix(60, 0), Selector: `rate({app="foo"} | unwrap foo[30s])`}},
},
// SUM(n=47, 61, 1) = 15 - there are 15 samples (from 47 to 61) matched from the generated series
// 15 / 30 = 0.5
promql.Vector{promql.Sample{Point: promql.Point{T: 60 * 1000, V: 0.5}, Metric: labels.Labels{labels.Label{Name: "app", Value: "foo"}}}},

[chaudum]

@ssncferreira Addressed your feedback in https://github.com/grafana/loki/pull/6412/files#diff-6f4083532ac476e6ab63b44775eb6356fba146db52e0a19759eff5045b92de2a

[KMiller-Grafana]

Docs in this PR look good to me.