3
\$\begingroup\$

I have written the following Perl script to monitor the hard drive performance of the particular process (say, for example, Chrome).

use strict;
use warnings;
use Spreadsheet::WriteExcel;

#@usage example perl script.pl 5 5 if($#ARGV!=1); 

my $login = (getpwuid $>);
die "must run as root" if $login ne 'root';

my $workbook = Spreadsheet::WriteExcel->new("Performance_report.xls");
my  $disk=$workbook->add_worksheet('disk');
$disk->write(0,0,"Time_interval",0);
$disk->write(0,1,"read/s",0);
$disk->write(0,2,"write/s",0);


#stores the time interval
my $time_interval=$ARGV[0];
#stores the no of iteration
my $test_count=$ARGV[1];
my $iter=0;
while($iter<$test_count)
{
   my @processes;
   @processes=`iotop -b -n 1 |grep chrome`;
   my $read_per_second=0;

   my $write_per_second=0;
   foreach(@processes)
   {
      my $read=0;
      my $write=0;
      while ($_ =~ /([\d\.]+)\s+[B|K]\/s+\s+([\d\.]+)/sg)
      {
          $read_per_second+=$1;
          $write_per_second+=$2;
      }
   }
   $disk->write($iter+1,0,$time_interval*($iter+1));
   $disk->write($iter+1,1,$read_per_second);
   $disk->write($iter+1,2,$write_per_second);
   sleep($time_interval);
   $iter++;
}
$workbook->close() or die "Cannot close the worksheet";

I have used the iotop tool in this script to monitor the disk performance.

sudo iotop -b -n 1 | grep chrome gives the following output:

3687 be/4 venkat      0.00 B/s    0.00 B/s  0.00 %  0.00 % chrome --type=zygote
4284 be/4 venkat      0.00 B/s    0.00 B/s  0.00 %  0.00 % chrome
3693 be/4 venkat      0.00 B/s    0.00 B/s  0.00 %  0.00 % chrome --type=zygote
3695 be/4 venkat      0.00 B/s    0.00 B/s  0.00 %  0.00 % chrome
3728 be/4 venkat      0.00 B/s    0.00 B/s  0.00 %  0.00 % chrome
5468 be/4 venkat      0.00 B/s    0.00 B/s  0.00 %  0.00 % chrome

I store the output to the array, process every value of the array, extract the second and third column value, recursively add the values of every thread of Chrome and store the final sum in the Excel sheet for every iteration. Is there any better way to do this using a Perl script?

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

in general this script seems to be straight forward.

But here are some suggestions for improvements:

First check for needed command line arguments and die early with a usage message if they are missing/incomplete. Alternatively you could supply defaults for time interval and iterations like this

#stores the time interval
my $time_interval=$ARGV[0] || 10;
#stores the no of iteration
my $test_count=$ARGV[1] || 50;

You also might want to remove the hard coded process name (chrome) and use a third command line parameter

#stores the process name
my $processname=$ARGV[2] || 'chrome';

and

@processes=`iotop -b -n 1 |grep $processname`;

The result of calls to the write method of Spreadsheet::WriteExcel should be checked for success (=0) and failures should be shown somewhere (either on the console and/or in some log file).

The processing of iotop output could be enhanced.

  1. Variables $read and $write are not needed.

  2. Each line needs to be matched only once (while -> if and no need for matching modifier g)

The regexs could be made more robust and written more readable.

  1. I defined the regex for the throughput part once in a variable ($throughputpattern) and used it twice afterwards in the match. This is a general principle (don't repeat yourself) for better maintainablility.
  2. the regex now uses the x modifier which allows whitespace in the pattern for better spacing and readability (but then you need to express whitespace to be matched explicitly with \s).
  3. I changed the accepted units of B and K to any capital letter. Who knows what throughput might be possible?

   my $throughputpattern = qr{\b (\d+\.\d+) \s+ [A-Z]/s \s+}x;
   foreach(@processes)
   {
      if ($_ =~ /$throughputpattern $throughputpattern/x)
      {
         $read_per_second+=$1;
         $write_per_second+=$2;
      }
   }
\$\endgroup\$
2
\$\begingroup\$

Overview

It is great that you:

  • Used strict and warnings
  • Leveraged other people's code by using the CPAN module

The other answer offers a lot of good advice, which I need not repeat. Here are some other adjustments for you to consider.

Special variables

It is good that you immediately check if the user is running as "root" before executing other code.

Since it hard for most people to remember all of Perl's special variables, like $>, it is a good practice to use the English module:

use English;

my $login = (getpwuid $EFFECTIVE_USER_ID);

Documentation

Instead of having a usage comment, you can add a summary of the purpose of the code with usage notes at the top of the file as plain old documentation (POD), something as simple as:

=pod

Monitor the hard drive performance of the particular process
(say, for example, Chrome).

Usage example:

    perl script.pl 5 5

=cut

Comments

Comments like this are really not needed since they just repeat what the code clearly does:

#stores the time interval
my $time_interval=$ARGV[0];

You could add a description of what the variable does in the context of the code, and what it's valid values are.

Input checking

You could check for valid values passed in on the command line. It would be best to do this before you write out the Excel file. It is also common to use shift to get data from the command line:

die 'Error: you must provide 2 numbers' if @ARGV < 2;

my $time_interval = shift;
my $test_count    = shift;

It would be a good idea to check if the values are numeric and fall within expected ranges.

Style

It is better to use whitespace around assignment operators:

my $disk = $workbook->add_worksheet('disk');

It is better to add a space after each comma and to use single quotes when you don't need to interpolate in strings:

$disk->write(0, 0, 'Time_interval', 0);

DRY

Since you use the expression $iter+1 multiple times, you can simplify this by initializing $iter to 1 instead of 0, then just use $iter without the +1:

my $iter = 1;

Style

I prefer to use the "quote executable" operator (qx) because I think it is easier to see in the code than backticks.

Also, you could simplify the code by combining the 3 lines:

my @processes;
@processes=`iotop -b -n 1 |grep chrome`;
foreach(@processes)

into 1:

for (qx(iotop -b -n 1 | grep chrome))

for is identical to foreach but easier to type.

If you always want to grab the 4th and 6th columns, then split might be a simpler solution than a regular expression:

for (qx(iotop -b -n 1 | grep chrome))
{
    my @cols = split;
    $read_per_second  += $cols[3];
    $write_per_second += $cols[5];
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.