search

go-graphite / carbonapi

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

Fix various issues with moving* functions #783

Closed carrieedwards closed 1 year ago

carrieedwards commented 1 year ago

This PR addresses multiple issues that were found with the moving* functions. These issues included:

  1. Moving* functions are intended to support two different types for the windowSize parameter: either a string containing an interval such as “1min” or “1h”, or an integer which represents a fixed number of points to be considered for the window. The CarbonAPI implementation contained a bug for the integer windowSize parameter in which the windowSize was not properly taken into consideration, and extraneous NaN values were added.
  2. The start time of the fetch request was being altered in the parser.Metrics() function, before the data was fetched, in order to take into consideration the specified time interval. However, it was only doing this if a string interval was supplied for the windowSize parameter; it ignored the case of an integer interval being specified, which corresponds to a specific number of points. In the case of a specific number of points being used for the window, the step time of the data has to be factored in, and this is not possible to know the step time before the data is fetched. Therefore, the moving* functions must re-fetch data to accommodate this
    1. Graphite-web ignores the first point of the re-fetched data. This issue in Graphite-web addresses why. CarbonAPI was not doing this, so the results were shifted.
  3. MovingMedian was not mapped to a consolidation function, so a query including the movingMedian function would fail.
  4. A window containing only NaN values could result in a window's sum can become negative, and when Mean() was called on the window, the negative sum being divided by w.Len() (with a value of 0, since this function returns the result of subtracting the number of NaN values from the length of the window's data) resulted in -Inf being returned. Graphite-web checks if there are any non-null values, and if there are none, a value of None is returned for that window. CarbonAPI will now return NaN in this case to match Graphite-web's behavior

There were also several issues with the start and end time not being properly altered.

See documentation for the moving* functions and the implementation in Graphite-web.