go-graphite / carbonapi

Implementation of graphite API (graphite-web) in golang
Other
309 stars 140 forks source link

Fix summarize() handling of intervals #768

Closed carrieedwards closed 1 year ago

carrieedwards commented 1 year ago

This PR is a rewrite of the summarize function in order to match Graphite web's behavior.

The main issue that was discovered is that when the interval parameter passed into summarize() was set to a value smaller than the fetched data's step between data points, it results were incorrect. The results were a copy of the data points for each series in the list. For example:

summarize(metric1,'5s') []*types.MetricData("metric1", []float64{1, 2, 3, 4, 5}, 10, now32)} // 10s step

The result was []float64{1, 2, 3, 4 5}. Issuing the same query with the same data in Graphite-web yielded results of [1, None, 2, Non, 3, None, 4, None, 5, None, None]. This is due to the interval being smaller than the step, and therefore some resulting values will be NaN.

This issue was corrected with a re-write of the summarize function, which now more closely matches Graphite web's implementation (see here and here.

Additionally, the stop time can be changed depending on how the intervals and stop time line up, so this was adjusted in the new version of the function.

Another issue was discovered, in which specifying 'count' for summarize (or smartSummarize) lead to unexpected results. This is because the consolidations SummarizeValues function's implementation for count counted all numbers, not just non-NaN numbers. To make this consistent with Graphite Web, the SummarizeValues implementation of count was updated to only count non-NaN values.

More testcases were added, including ones that cover the situation where the specific interval is smaller than the fetched data's step, as well as test cases translated from Graphite web for summarize().