On 08/31/2009 12:49 PM, Bjoern Geuken wrote:
> ref: refs/heads/master
> commit 7fd7a8616e46b77b97d668b6782283864d32295b
> Author: Bjoern Geuken <[email protected]>
> Date: Mon Aug 31 12:49:45 2009 +0200
>
> status module: fixed evaluation of intervals for metrics
> ---
> plugins/status/lib/status.rb | 33 ++++++++++++------------------
> plugins/status/test/unit/status_test.rb | 3 +-
> 2 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/plugins/status/lib/status.rb b/plugins/status/lib/status.rb
> index 8628dde..58b7344 100644
> --- a/plugins/status/lib/status.rb
> +++ b/plugins/status/lib/status.rb
> @@ -9,9 +9,8 @@ class Status
> :health_status,
> :metrics,
> :limits
> -#=begin
> +
> def to_xml(options = {})
> -# puts @limits.inspect
> xml = options[:builder] ||= Builder::XmlMarkup.new(options)
> xml.instruct! unless options[:skip_instruct]
>
> @@ -20,7 +19,7 @@ class Status
> data.each {|metric, data|
> xml.metric(:name => metric, :metricgroup => metric_group) do
> xml.starttime(@data[metric_group][metric]["starttime"])
> - xml.interval(5)
> + xml.interval(@data[metric_group][metric]["interval"])
> data.each {|label,data|
> unless label == "starttime" or label == "interval"
> xml.label(:type => "hash", :name => label) do
> @@ -201,43 +200,37 @@ class Status
> end
> unless labels.blank?
> # set values for each label and time
> + # evaluates interval and starttime once for each metric (not each
> label)
> + nexttime = 9.9e+99
> + result["starttime"] = 9.9e+99
I suggest use some constant for this number because it is quite
confusing (I think that it is some initial value, but maybe it is error
indicator or some default. Constant give behavior to number and it is
then easier to change it and test values on this value
> lines.each do |l| # each time
> unless l.blank?
It is just my recomendation, but if you do big "if" statement in code
and indent whole loop body, I think that if you just use
next if l.blank?
at begin of loop it increase readiness. (the best sulution is to write
shorted functions or refactor it to separated parts - I recommend use
reek or roodi to check your coding style)
> if l =~ /\d*:\D*/
It looks like duplicite check, you check blank and then regex which is
not blank, is really necessary to test blankness? and I miss at least
some logging about bad input value.
> pair = l.split ":"
> values = pair[1].split " "
> - #puts pair[1]
> column = 0
> values.each do |v| # each label
> - result["starttime"] ||= pair[0]
> - result["starttime"] = pair[0] if pair[0].to_i <
> result["starttime"].to_i
> - nexttime =
> + if result["interval"].nil?
> + if pair[0].to_i < result["starttime"].to_i
> + result["starttime"] = pair[0].to_i
> + elsif pair[0].to_i < nexttime and pair[0].to_i >
> result["starttime"].to_i
> + nexttime = pair[0].to_i
> + end
> + end
>
> if v != "nan" #store valid values only
> result["#{labels[column]}"] ||= Hash.new
> -# result["#{labels[column]}"].merge!(v)
> result["#{labels[column]}"].merge!({"#{pair[0].chomp(": ")}"
> => v})
I don't understand why you chomp there, as you have only digits in
pair[0] ( regex ensure this ).
Second really confusing code on this line is merge. It is really useless.
if you use
result["#{labels[column]}"][pair[0]] = v
you get same result.
Another think is that you reusing "#{labels[column]}".
At first you can save it samewhere like label = "#{labels[column]}"
At second I think that this string is really confusing, why you don't
use just labels[column] ? and if it is not String, then add .to_s ?
> - #result["#{labels[column]}"].merge!({"T_#{pair[0].chomp(":
> ")}" => v})
> column += 1
> else
> result["#{labels[column]}"] ||= Hash.new
> -# result["#{labels[column]}"].merge!("invalid")
> result["#{labels[column]}"].merge!({"#{pair[0].chomp(": ")}"
> => "invalid"})
Same as above and you don't increase column, so if you get invalid value
and then get valid value you with same pair[0] you overwrite invalid value.
Is it intended to increase column only for valid data? It looks little
confusing for me, as if status is invalid (cannot fetch data etcc) then
I think it should show or hide this column, but next measure should be
in next column not?
> end
> end
> end
> end
> end
> - #setting the limits
> -# result.each do |key, value|
> -# path = rrdfile[datapath.length+1..rrdfile.length-1].chomp('.rrd')
> -# path +="/" + key if key!="value" #do not take care about the value
> flag
> -# path = path.tr('-','_')
> -# if @limits.has_key?(path)
> -# result[key] ||= Hash.new
> -# result[key].merge!({"limit" => @limits[path] })
> -# end
> -# end
> + result["interval"] = nexttime.to_i - result["starttime"].to_i if
> result["interval"].nil?
testing for nil? is automatic so you can use
result["interval"] = nexttime.to_i - result["starttime"].to_i unless
result["interval"]
If you have any question feel free to ask.
--
Josef Reidinger
YaST team
maintainer of perl-Bootloader, YaST2-Repair, webyast modules language
and time
--
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
