[Ovirt-devel] [PATCH server] Add max/min methods to stats results, various other fixes to stats

[Jim Meyering]

Steve Linabery <slinabery at redhat.com> wrote:
>>From df0bb94ed5549ab6826da255f9acca5e414938aa Mon Sep 17 00:00:00 2001
> From: Steve Linabery <slinabery at redhat.com>
> Date: Wed, 17 Sep 2008 15:22:39 -0500
> Subject: [PATCH] Add max/min methods to stats results, various other fixes to stats
>
> ---
>  src/app/util/stats/Stats.rb         |   64 +++++++++++++++++++++++++++++++----
>  src/app/util/stats/StatsDataList.rb |   28 +++++++++++++--
>  src/app/util/stats/statsTest.rb     |   38 +++++++++++++-------
>  3 files changed, 107 insertions(+), 23 deletions(-)

ACK, comments below.

> diff --git a/src/app/util/stats/Stats.rb b/src/app/util/stats/Stats.rb
> index f6ced4b..b434232 100644
> --- a/src/app/util/stats/Stats.rb
> +++ b/src/app/util/stats/Stats.rb
> @@ -29,6 +29,8 @@ require 'util/stats/StatsRequest'
>
>  def fetchRollingAve?(rrdPath, start, endTime, interval, myFunction, lIndex, returnList, aveLen=7)
>     final = 0
> +   my_min = 0
> +   my_max = 0
>
>     #  OK, first thing we need to do is to move the start time back in order to
>     #  have data to average.
> @@ -65,19 +67,32 @@ def fetchRollingAve?(rrdPath, start, endTime, interval, myFunction, lIndex, retu
>              final += rdata
>           end
>           final = (final / aveLen )
> +
> +         # Determine min / max to help with autoscale.
> +         unless final.is_a? (Float) && final.nan?
> +           my_min = [my_min, final].min
> +           my_max = [my_max, final].max
> +         end
>           returnList.append_data( StatsData.new(fstart + interval * ( i - indexOffset), final ))
>
>           # Now shift the head off the array
>           roll.shift
>        end
>     end
> -
> +
> +   # Now add the min / max to the lists
> +   returnList.set_min_value(my_min)
> +   returnList.set_max_value(my_max)
> +
>   return returnList
>  end
>
>
>  def fetchRollingCalcUsedData?(rrdPath, start, endTime, interval, myFunction, lIndex, returnList, aveLen=7)
>
> +   my_min = 0
> +   my_max = 0
> +
>     # OK, first thing we need to do is to move the start time back in order to have data to average.
>
>     indexOffset = ( aveLen / 2 ).to_i
> @@ -120,12 +135,22 @@ def fetchRollingCalcUsedData?(rrdPath, start, endTime, interval, myFunction, lIn
>              final += rdata
>           end
>           final = (final / aveLen)
> +
> +         # Determine min / max to help with autoscale.
> +         unless final.is_a? (Float) && final.nan?
> +           my_min = [my_min, final].min
> +           my_max = [my_max, final].max
> +         end
>           returnList.append_data( StatsData.new(fstart + interval * ( i - indexOffset), final ))
>           # Now shift the head off the array
>           roll.shift
>        end
>     end
>
> +   # Now add the min / max to the lists
> +   returnList.set_min_value(my_min)
> +   returnList.set_max_value(my_max)
> +
>   return returnList
>  end
>
> @@ -137,6 +162,9 @@ def fetchCalcUsedData?(rrdPath, start, endTime, interval, myFunction, lIndex, re
>     #  We also need to handle NaN differently
>     #  Finally, we need to switch Min and Max
>
> +   my_min = 0
> +   my_max = 0
> +
>     lFunc = "AVERAGE"
>     case myFunction
>        when "MAX"
> @@ -155,13 +183,23 @@ def fetchCalcUsedData?(rrdPath, start, endTime, interval, myFunction, lIndex, re
>     data.each do |vdata|
>        i += 1
>        value = vdata[lIndex]
> -         value = 100 if value.nan?
> -         if ( value > 100 )
> -            value = 100
> -         end
> -         value  =  100 - value
> +      value = 100 if value.nan?
> +      if ( value > 100 )
> +         value = 100
> +      end
> +      value  =  100 - value

No need for the value.nan? test here, since it's handled just above,
but it may be just as well to leave it for consistency with the
other, similar uses.

> +      # Determine min / max to help with autoscale.
> +      unless value.is_a? (Float) && value.nan?
> +        my_min = [my_min, value].min
> +        my_max = [my_max, value].max
> +      end
>        returnList.append_data( StatsData.new(fstart + interval * i, value ))
>     end

A thought for later (not to hold up this review):
rather than imposing that repetitive (and relatively subtle)
NaN test everywhere, is there some place central where you
deal with any incoming NaN values up front so code like this
can assume sanitized values?

IMHO, that'd be more maintainable.  Otherwise, every comparison
involving those data will have to think hard how to handle NaNs.