(SLV-653) Add a script for generating system stats #28

RandellP · 2019-10-25T17:35:12Z

The script is intended to be used as part of an optional feature to get system stats the same way other stats are gathered and managed.
It uses sar which will poll every x seconds and generate an average, which can then be captured into a file at the same interval as the puppet-metrics-collector polling interval. This keeps the total data down while not missing things that cause short duration spikes (like compiles that last 6 seconds). Initial use will be in performance testing.

RandellP · 2019-10-25T17:37:37Z

@billclaytor @johnduarte I just put you two on as reviewers for now. Please don't merge. After you two approve I will add Reid.

johnduarte · 2019-10-28T20:14:35Z

files/gen_sys_stats.rb

+    # @author Randell Pelak
+    #
+    # @param [integer] polling_interval Time in seconds between calls to poll the system for data.
+    # @param [integer] file_interval Tim in seconds between the creation of each output file.


s/Tim /Time /

johnduarte · 2019-10-28T20:18:21Z

files/gen_sys_stats.rb

+      @metrics_dir = metrics_dir
+      @verbose = verbose
+
+      @hostname = %x[hostname].strip


Will this work as expected on the primary master? I thought the primary master was being defined at 127.0.0.1 by the metrics collector. This will use the hostname for that node instead.

Reid pushed a change to fix the 127.0.0.1 thing... he considered that a bug. I don't think it got released yet though. The change was in the hosts_with_profile function there were using to get the hosts and put them in the params. I thought about making host a parameter to the script but decided it shouldn't really matter what the dir is called as long as it is mappable to the host. https://github.com/puppetlabs/puppetlabs-puppet_metrics_collector/pull/26/files

johnduarte · 2019-10-28T20:20:03Z

files/gen_sys_stats.rb

+
+      @hostname = %x[hostname].strip
+      puts "Hostname is: #{@hostname}" if @verbose
+      FileUtils.mkdir_p(metrics_dir) unless File.directory?(metrics_dir)


⛏️ Shouldn't the instance variable be used here since it has been assigned?

johnduarte · 2019-10-28T20:27:29Z

files/gen_sys_stats.rb

+    #
+    # @return [string] raw output from sar
+    def run_sar
+      times_to_poll = ((@file_interval / @polling_interval) + 0.5).to_i


You should use the ceil method rather than fudging the numbers

times_to_poll = (@file_interval / @polling_interval).ceil

use round. https://apidock.com/ruby/Float/round

johnduarte · 2019-10-29T18:22:36Z

files/gen_sys_stats.rb

+    #
+    # @return [boolean]
+    def sar_output_valid?(sar_output)
+      last_line = sar_output.split("\n")[-1]


Refactor suggestion: You are performing string manipulation to get data out of the sar output in several methods. You already have code to structure the data theconvert_sar_output_to_json method. I suggest breaking out the code that creates the metrics_data hash into its own method combined with the string manipulation. If the sar output changes in the future all code related to parsing is from string output should be encapsulated in one place.

This would make this method unnecessary (raise error during hash creation if headers != averages). The hash can then be passed to the get_* methods.

yeah, I didn't like the way this worked out. Thinking about it now, I am having trouble coming up with method names that make sense.
I feel like I want to write get_sar_data_hash... have it call run_sar, and then convert... with convert checking for errors and raising if needed. This of course eliminates "sar_output_valid?" But does that sound right? Feels like get_sar_data_hash doesn't communicate that it will actually run sar. Guess I could go with run_sar_and_get_data_hash???

I tried out somethings, didn't like them, and tried some more. I think I found something that works better. But the convert_sar_output_to_json ends up being a bit of a wall of text... but it isn't very complex so that might be okay. Tell me what you think.

johnduarte · 2019-10-29T18:27:16Z

files/gen_sys_stats.rb

+    def convert_sar_output_to_json(sar_output, time_stamp_obj)
+      header = get_header_fields(sar_output)
+      averages = get_averages_fields(sar_output)
+      hostkey = @hostname.gsub('.', '-')


Why change the hostname? The key is a string, so the '.' characters should not cause any issue with Ruby or JSON.

this was to be consistent with the rest of PMC. The value is actually the default tag if the data gets loaded into influxdb... so maybe it cares... dunno for sure, but wanted it to be consistent.

johnduarte · 2019-10-29T18:29:48Z

files/gen_sys_stats.rb

+    # @author Randell Pelak
+    #
+    # @return [void]
+    def go


Maybe "generate_system_metrics"?

johnduarte · 2019-10-29T18:41:28Z

files/gen_sys_stats.rb

+
+  options = {}
+
+  OptionParser.new do |opts|


This takes up many lines. I am no expert here, but I am wondering if there is another library that could reduce the CLI boilerplate.
Puppet uses one called trollop which has been renamed to optimist. I have also bumped into others using thor.

I think it is kind of a path of least resistance at this point. We have used it in several places and are at least familiar with it. We could make a ticket to investigate others at some point...

RandellP · 2019-10-29T21:31:14Z

script name and class should maybe be the same

RandellP · 2019-10-31T17:03:09Z

@johnduarte Ready for another look

johnduarte · 2019-10-31T17:45:11Z

files/generate_system_metrics

+      hostkey = @hostname.gsub('.', '-')
+      dataset = {'time_stamp_obj' => time_stamp_obj.utc.iso8601, 'servers' => {}}
+      #combine the arrays into a hash and convert into json
+      metrics_data = Hash[header.zip(averages)]


This is passing raw STDOUT string data to three methods. It would be preferable to encapsulate the conversion of the sar string output to a defined data set. That way if the output format of sar changes in the future only one method would need to be updated.

Suggestion:

# @return [hash] metrics_data def parse_sar_output(sar_output) sar_data = {} sar_arr = sar_output.split(/\n+|\r+/).reject(&:empty?).map { |line| line.split } averages = sar_arr.find { |e| e.include? "Average:" } headers = sar_arr.find { |e| e.include? "kbmemfree" } metrics_data = Hash[headers.zip(averages)] end

Then pass the data to convert_metrics_to_json.

@johnduarte hmmm. I played around with that. I run into the problems getting useful error messages out. So it got a bit longer. But it encapsulated all the processing in one method. See what you think.

johnduarte · 2019-10-31T22:05:53Z

files/generate_system_metrics

+    def parse_sar_output(sar_output)
+      sar_output_arr = sar_output.split(/\n+|\r+/).reject(&:empty?).map { |line| line.split }
+
+      headers_line = sar_output_arr.find { |e| e.include? "%user" }


What is "%user"? This is not included in my version of sar.

This is what I see
[root@ip-10-227-0-89 ~]# sar 1 3
Linux 3.10.0-957.1.3.el7.x86_64 (ip-10-227-0-89.amz-dev.puppet.net) 11/01/2019 x86_64 (8 CPU)

04:47:43 PM CPU %user %nice %system %iowait %steal %idle
04:47:44 PM all 0.63 0.00 0.13 0.00 0.00 99.25
04:47:45 PM all 0.12 0.00 0.00 0.00 0.00 99.88
04:47:46 PM all 0.75 0.00 0.12 0.00 0.00 99.12
Average: all 0.50 0.00 0.08 0.00 0.00 99.42
[root@ip-10-227-0-89 ~]#

the kbmemfree you posted before sounded like you were doing the mem version "sar -r 1 3" But the point is good... cause the script supports both, and would have failed to find %user in the mem version... So doing your update for the reverse zip is critical.

actually, the reverse thing doesn't solve the %user... I will have to choose what to look for based on what metrics type is being run

sar_output_arr.find { |e| e.include? "%user" or e.include? "kbmemfree" }

I did it in a more wordy way. and tied it to @metric_type

johnduarte · 2019-10-31T22:41:48Z

files/generate_system_metrics

+                                  "\nFull output:\n#{sar_output}"
+      raise(sar_error_header_mismatch) unless headers.count == averages.count
+      #combine the arrays into a hash
+      Hash[headers.zip(averages)]


There are magic numbers for array segments and much validation. The later is fine. The former is undesirable.

There appear to be two issues that you are addressing.

The headers and the averages start with some BS you don't want.

Due to the BS, there is a risk of the fields being mismatched.

Given that all of the BS is at the beginning of the headers and the averages field lists, I think that zipping these fields in reverse and tossing away any results that do not have a numeric value solves both problems.

If no data arrays are found for the desired lines (e.g. headers, averages), then the call to zip will throw an exception without checking them beforehand.

With those things in mind, I am wondering if the following refactor may simplify things. Please feel free to disregard if it does not or is not conservative enough.

headers_line = ... # as above averages_line = ... # as above # This will zip the data in reverse order to ensure a correct mapping. # Since it is being converted to a hash, we don't care about order afterwards. # If either the headers_line or averages_line is nil, then an error will be raised sar_data = Hash[headers_line.reverse.zip(averages_line.reverse)] # Now we throw away any data whose value is not numeric. sar_data = sar_data.select{ |k,v| v =~ /\A[-+]?[0-9]*\.?[0-9]+\Z/ } return sar_data

great idea, I really like it. implemented.

RandellP · 2019-11-01T17:16:07Z

@johnduarte ready for another look

johnduarte

🎉 no more magic numbers!

billclaytor · 2019-11-04T17:28:14Z

files/generate_system_metrics

+  DESCRIPTION
+
+  DEFAULTS = <<-DEFAULTS
+    The following defaults values are used if the options are not specified:


Suggested change

The following defaults values are used if the options are not specified:

The following default values are used if the options are not specified:

billclaytor · 2019-11-04T17:33:40Z

files/generate_system_metrics

+
+  polling_interval = options[:polling_interval] || POLLING_INTERVAL_DEFAULT
+  file_interval = options[:file_interval] || FILE_INTERVAL_DEFAULT
+  metric_type= options[:metric_type] || METRIC_TYPE_DEFAULT


Suggested change

metric_type= options[:metric_type] || METRIC_TYPE_DEFAULT

metric_type = options[:metric_type] || METRIC_TYPE_DEFAULT

billclaytor · 2019-11-04T17:53:15Z

files/generate_system_metrics

+      end
+    end
+
+


Is this extra line intentional?

nope, removing

files/generate_system_metrics

billclaytor · 2019-11-04T19:43:53Z

files/generate_system_metrics

+      file_path = "#{dirname}/#{filename}"
+      FileUtils.mkdir_p(dirname) unless File.directory?(dirname)
+      puts "Creating json file: #{file_path}" if @verbose
+      File.open(file_path, 'w') do |file|


Would the 'short version' be better here or is it better to use the longer form?

File.write(file_path, json_dataset)

I often like the long version, but in this case I don't think it adds anything... so changing to the short version

RandellP · 2019-11-05T17:48:06Z

@billclaytor updated based on your feedback... take another look, thanks.

billclaytor

Awesome!

The script is intended to be used as part of an optionial feature to gate system stats the same way other stats are gathered and managed. It uses sar which will poll every x seconds and generate an average, which can then be captured into a file at the same interval as the puppet-metrics-collector polling interval. This keeps the total data down while not missing things that cause short duration spikes (like compiles that last 6 seconds). Initial use will be in performace testing. Had to disable GetText/DecorateString cop for the entire script in the rubocop.yml because inline disables aren't working for some reason. And rubocop is parsing for ruby 2.1 where the fix works, but the same fix doesn't work with later versions of ruby, like the one installed by puppet...

jarretlavallee · 2019-11-06T23:13:46Z

@RandellP This looks like a great addition. Is there supposed to be a sar_metric.pp with this as well?

RandellP · 2019-11-06T23:37:50Z

@jarretlavallee that will be in the next PR... :) Trying to keep them small so our team reviewing is easier... We did several laps on this one to get it solid before sending it to you and Reid.

RandellP · 2019-11-06T23:39:00Z

BTW, John and Bill both approved... but it reset their status when I pushed up the changes to the comments that reid asked for. So it sounds like it is ok to merge by the next person who sees this. :)

reidmv

This is piecemeal, but I'm okay with merging it now since it doesn't conflict with existing content, and reviewing the part that makes use of it whenever you have that ready.

RandellP requested review from johnduarte and billclaytor October 25, 2019 17:35

RandellP force-pushed the SLV-653 branch from 99c4bd0 to a3da890 Compare October 25, 2019 18:37

johnduarte suggested changes Oct 29, 2019

View reviewed changes

RandellP force-pushed the SLV-653 branch 2 times, most recently from f628765 to c801b23 Compare October 31, 2019 16:00

johnduarte reviewed Oct 31, 2019

View reviewed changes

RandellP force-pushed the SLV-653 branch from c801b23 to 4b9eb3a Compare October 31, 2019 19:22

johnduarte reviewed Oct 31, 2019

View reviewed changes

johnduarte suggested changes Oct 31, 2019

View reviewed changes

RandellP force-pushed the SLV-653 branch from 4b9eb3a to cd06dfd Compare November 1, 2019 17:11

johnduarte previously approved these changes Nov 1, 2019

View reviewed changes

billclaytor reviewed Nov 4, 2019

View reviewed changes

files/generate_system_metrics Show resolved Hide resolved

billclaytor reviewed Nov 4, 2019

View reviewed changes

RandellP dismissed johnduarte’s stale review via b730f15 November 5, 2019 17:46

RandellP force-pushed the SLV-653 branch from cd06dfd to b730f15 Compare November 5, 2019 17:46

billclaytor previously approved these changes Nov 5, 2019

View reviewed changes

RandellP requested a review from reidmv November 5, 2019 19:56

RandellP mentioned this pull request Nov 5, 2019

(SLV-672) Add system class to manage system metric #30

Merged

RandellP dismissed billclaytor’s stale review via e64505b November 6, 2019 23:06

RandellP force-pushed the SLV-653 branch from b730f15 to e64505b Compare November 6, 2019 23:06

RandellP requested a review from jarretlavallee November 6, 2019 23:06

RandellP force-pushed the SLV-653 branch from e64505b to c3a948a Compare November 6, 2019 23:09

reidmv approved these changes Nov 7, 2019

View reviewed changes

jarretlavallee merged commit 627c17c into master Nov 7, 2019

johnduarte deleted the SLV-653 branch November 8, 2019 15:27

MartyEwings added the feature label May 27, 2021

	The following defaults values are used if the options are not specified:
	The following default values are used if the options are not specified:

	metric_type= options[:metric_type] \|\| METRIC_TYPE_DEFAULT
	metric_type = options[:metric_type] \|\| METRIC_TYPE_DEFAULT

(SLV-653) Add a script for generating system stats #28

(SLV-653) Add a script for generating system stats #28

Conversation

RandellP commented Oct 25, 2019 • edited Loading

RandellP commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandellP commented Oct 29, 2019

RandellP commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandellP commented Nov 1, 2019

johnduarte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandellP commented Nov 5, 2019

billclaytor left a comment

Choose a reason for hiding this comment

jarretlavallee commented Nov 6, 2019

RandellP commented Nov 6, 2019

RandellP commented Nov 6, 2019

reidmv left a comment

Choose a reason for hiding this comment

RandellP commented Oct 25, 2019 •

edited

Loading