Stefan Schubert write:
> ref: refs/heads/master
> commit 4e4d3e055a49b1cf76c2082873b9674b2d8f13eb
> Author: Stefan Schubert <[email protected]>
> 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. <[email protected]>
> # @author Stefan Schubert <[email protected]>
> #
> -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: [email protected]
For additional commands, e-mail: [email protected]
