Mailinglist Archive: yast-devel (163 mails)

< Previous Next >
[yast-devel] Re: [yast-commit] <rest-service> master : status module: fixed evaluation of intervals for metrics
  • From: Josef Reidinger <jreidinger@xxxxxxx>
  • Date: Tue, 01 Sep 2009 08:48:41 +0200
  • Message-id: <4A9CC3C9.4030006@xxxxxxx>
On 08/31/2009 12:49 PM, Bjoern Geuken wrote:
ref: refs/heads/master
commit 7fd7a8616e46b77b97d668b6782283864d32295b
Author: Bjoern Geuken <bgeuken@xxxxxxx>
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
def to_xml(options = {})
-# puts @limits.inspect
xml = options[:builder] ||=
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.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
unless labels.blank?
# set values for each label and time
+ # evaluates interval and starttime once for each metric (not each
+ 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 <
- 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 >
+ nexttime = pair[0].to_i
+ end
+ end

if v != "nan" #store valid values only
result["#{labels[column]}"] ||=
-# 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
result["#{labels[column]}"] ||=
-# 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?

- #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
-# path ='-','_')
-# if @limits.has_key?(path)
-# result[key] ||=
-# result[key].merge!({"limit" => @limits[path] })
-# end
-# end
+ result["interval"] = nexttime.to_i - result["starttime"].to_i if

testing for nil? is automatic so you can use
result["interval"] = nexttime.to_i - result["starttime"].to_i unless

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@xxxxxxxxxxxx
For additional commands, e-mail: yast-devel+help@xxxxxxxxxxxx

< Previous Next >
This Thread
  • No further messages