Mailinglist Archive: yast-devel (77 mails)

< Previous Next >
[yast-devel] Re: [yast-commit] <rest-service> master : cleanup code; bring metrix to the level of status
  • From: Josef Reidinger <jreidinger@xxxxxxx>
  • Date: Tue, 22 Dec 2009 11:47:32 +0100
  • Message-id: <200912221147.32967.jreidinger@xxxxxxx>
Stefan Schubert write:
ref: refs/heads/master
commit 4e4d3e055a49b1cf76c2082873b9674b2d8f13eb
Author: Stefan Schubert <schubi@xxxxxxx>
Date: Tue Dec 22 11:03:14 2009 +0100

cleanup code; bring metrix to the level of status
---
.../status/app/controllers/metrics_controller.rb | 1 -
plugins/status/app/models/metric.rb | 57
+++++++++++++++++++-
2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/plugins/status/app/controllers/metrics_controller.rb
b/plugins/status/app/controllers/metrics_controller.rb
index c699898..d39205f 100644
--- a/plugins/status/app/controllers/metrics_controller.rb
+++ b/plugins/status/app/controllers/metrics_controller.rb
@@ -1,6 +1,5 @@
include ApplicationHelper

-require 'scr'
require 'metric'
require 'uri'

diff --git a/plugins/status/app/models/metric.rb
b/plugins/status/app/models/metric.rb
index f3e0919..ef8ec02 100644
--- a/plugins/status/app/models/metric.rb
+++ b/plugins/status/app/models/metric.rb
@@ -5,7 +5,26 @@
# @author Duncan Mac-Vicar P. <dmacvicar@xxxxxxx>
# @author Stefan Schubert <schubi@xxxxxxx>
#
-require 'yaml'
+require 'exceptions'
+
+class CollectdOutOfSyncError < BackendException

Hi, few notes :)
I know that I also do it, but when you clean code I suggest to move this
exception to lib directory of plugin (so
plugins/status/lib/collectd_out_of_sync_error.rb to work rails autoload).

+ def initialize(timestamp)
+ @timestamp = timestamp
+ super("Collectd is out of sync.")
+ end
+
+ def to_xml
+ xml = Builder::XmlMarkup.new({})
+ xml.instruct!
+
+ xml.error do
+ xml.type "COLLECTD_SYNC_ERROR"
+ xml.description "Collectd is out of sync. Status information can be
expected at #{Time.at(@timestamp.to_i).ctime}."
+
+ xml.timestamp @timestamp
+ end
+ end
+end

#
# This class acts as a model for metrics provided by collectd
@@ -30,6 +49,7 @@ require 'yaml'
# end
#
class Metric
+ require 'yaml'

COLLECTD_BASE = "/var/lib/collectd"

@@ -41,6 +61,7 @@ class Metric
# convenience, plugin and instance
attr_reader :plugin_full
attr_reader :type_full
+ attr_reader :config

# like identifier, but to be used as a REST id
# so / are replaced by +
@@ -123,6 +144,18 @@ class Metric
# these values can be extracted from the file but we cache
# them
@host, @plugin, @plugin_instance, @type, @type_instance =
Metric.parse_file_path(path)
+
+ #find the correct plugin path for the config file
+ plugin_config_dir = "#{RAILS_ROOT}/config" #default
+ Rails.configuration.plugin_paths.each do |plugin_path|
+ if File.directory?(File.join(plugin_path, "status"))
+ plugin_config_dir = plugin_path+"/status/config"
+ Dir.mkdir(plugin_config_dir) unless
File.directory?(plugin_config_dir)

^^^
Attention: This doesn't work! yastws doesn't have write permissions to itself
so you cannot create directory below RAILS_ROOT! (I already have same problem
with writing css cache)

+ break
+ end
+ end
+ #reading configuration file
+ @conf = YAML.load(File.open(File.join(plugin_config_dir,
"status_configuration.yaml"))) if File.exists?(File.join(plugin_config_dir,
"status_configuration.yaml"))
^^^^
I suggest store to temp variable File.join which you repeat. It is not DRY,
effective and you have unnecessary long line:

conf_file = File.join(plugin_config_dir, "status_configuration.yaml")
@conf = YAML.load(File.open(conf_file)) if File.exists? conf_file

end

# parses the host, plugin, plugin_instance, type and type_instance
@@ -226,6 +259,13 @@ class Metric
xml.type type
xml.type_instance type_instance

+ if @conf and @conf.has_key?(id)
You can use try ( but I am not sure if it improve readability so use what you
find better)
if @conf.try(:has_key,id)
+ xml.limits() do
+ xml.max(@conf["#{id}"]["max"])
+ xml.min(@conf["#{id}"]["min"])
^^^
I don't understand why you use "#{id}"...if you want string just use id.to_s
... if symbol, then use id.to_sym
+ end
+ end
+
# serialize data unless it is disabled
unless opts.has_key?(:data) and !opts[:data]
metric_data = data(data_opts)
@@ -242,6 +282,12 @@ class Metric
end

end
+
+ #get last entry of rrd database
+ def self.last_db_entry(file)
+ `/bin/sh -c "LC_ALL=C rrdtool last #{file}"`
^^^
I don't look at whole code, but you must be really sure, that file cannot be
compromited otherwise it is security problem. Also be sure, then file doesn't
contain special characters. so at least '#{file}' should be used as not much
file has ' inside :)
+ end
+

# runs the rddtool on file with start time and end time
#
@@ -250,6 +296,15 @@ class Metric
#
# default last 5 minutes from now
def self.run_rrdtool(file, opts={})
+
+ #checking if collectd is running
+ ServiceNotRunning.new('collectd') unless collectd_running?
^^^^
I think there is missing raise - test, test,test ;) if you change behavior,
test it at first (test-driven development ;)
+
+ #checking if systemtime is BEHIND the last entry of collectd.
+ #If yes, no data will be available.
+ timestamp = last_db_entry(file)
+ raise CollectdOutOfSyncError.new(timestamp) if Time.now <
Time.at(timestamp.to_i)
+
stop = opts.has_key?(:stop) ? opts[:stop] : Time.now
start = opts.has_key?(:start) ? opts[:start] : stop - 300



--
Josef Reidinger
YaST team
maintainer of perl-Bootloader, YaST2-Repair, webyast
(language,time,basesystem,ntp)
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
For additional commands, e-mail: yast-devel+help@xxxxxxxxxxxx

< Previous Next >
Follow Ups