On 08/31/2009 12:49 PM, Bjoern Geuken wrote:

ref: refs/heads/master commit 7fd7a8616e46b77b97d668b6782283864d32295b Author: Bjoern Geuken <bgeuken@suse.de> 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: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org