Loop through all virtualhost log files and run goaccess on each file

Question

So I have multiple websites running under apache2 virtualhost - and I wanted to use GoAccess to process the access.log for each site.

The directory structure is like so:

/home/www/site1/html
/home/www/site1/log
/home/www/site1/stats

/home/www/site2/html
/home/www/site2/log
/home/www/site2/stats

Some sites contain two different access.log files -

ssl.access.log for SSL

access.log for non-SSL

These are located in the /log directory of each site

I wanted a cronjob to run every night to process the stats with GoAccess, but I didn't want to write multiple lines of nearly duplicate commands.

I've never written a bash script before, and so I do not know if this is the most efficient way of doing things.

Each report that is generated, needs the month/year in it, so each night it gets overwritten with that months latest stats.

the reports are outputted in the stats directory of each site, in the following format

yyyy-mm.html
sslyyyy-mm.html

The Script

#!/bin/bash

# find all log files which match ess.log
LOG_FILES="/home/www/*/log/*ess.log"

# set the date format
NOW=$(date +"%Y-%m")

# loop through each log file
for f in $LOG_FILES
do

  # drop back from /home/www/site/log to /home/www/site
  path=`dirname $f`
  path=`dirname $path`

  # get the current log filename
  filename=`basename $f`

  # if /home/www/site/stats does not exist - create it
  if [ ! -d "$path/stats" ]; then
    mkdir "$path/stats"
  fi


 # get the first part of the log filename
 prefix=(${filename//./ })

 # if its equal to access, then it's not ssl log, so remove the prefix
 if [ $prefix == 'access' ]; then
   prefix=''
 fi

  # run the goaccess process
  goaccess -f $f --date-format=%d/%b/%Y --log-format='%h %^[%d:%^] "%r" %s %b "%R" "%u"' -a > "$path/stats/$prefix$NOW.html" 

done

I know this is a fairly simple task, but as I have not any experience specifically in this it would be great to know where I could improve this.

200_success · Accepted Answer · 2014-12-13 04:40:39Z

The script has problems with quoting and wildcard expansion. The script could break in unexpected ways if there are spaces or shell metacharacters in the paths. In general, when writing shell scripts, anytime you want to write $variable, you should probably be writing "$variable" instead, expanding the variable in double-quoted context. The only unquoted variable in this script should be $LOG_FILES, because you do want wildcard expansion to occur there.

I think you are manipulating variables too much. For each $f, you are interested in the directory that will contain the output and the filename of the output. I think that the following script would be shorter and more obvious.

LOG_FILES=/home/www/*/log/*access.log
NOW=$(date +%Y-%m)

for f in $LOG_FILES ; do
  path=`dirname "$f"`/../stats
  mkdir -p "$path"                                   # Creates directory as necessary

  case `basename "$f"` in
    ssl.access.log) output="$path/ssl$NOW.html" ; ;;
        access.log) output="$path/$NOW.html"    ; ;;
                 *) exit 1                           # Shouldn't be possible
  esac

  goaccess -f "$f" --date-format=%d/%b/%Y \
           --log-format='%h %^[%d:%^] "%r" %s %b "%R" "%u"' -a > "$output"
done

Thank you very much for looking over this - the way you've written it certainly makes sense to me — DannyTheDev
– DannyTheDev, Commented Dec 14, 2014 at 20:21

Etan Reisner · Accepted Answer · 2014-12-12 18:52:18Z

It likely doesn't matter but I would replace

LOG_FILES="/home/www/*/log/*ess.log"

with

LOG_FILES="/home/www/*/log/*access.log"

just to be more clear about what you are trying to process (and just in case you ever get anything else that matches *ess.log by accident).

You can replace

if [ ! -d "$path/stats" ]; then
  mkdir "$path/stats"
fi

with mkdir -p if you wanted to.

You could replace

path=`dirname $f`
path=`dirname $path`

with

path=$(dirname "$(dirname "$f")")

too. (Even if not you should quote "$f" in the original for safety.)

The only other comment I have is that if you are concerned that goaccess might not complete successfully on any given run then you might to output to a temporary file and only rename to the final location when goaccess returns success (0).

Thanks for reviewing my code, the path confuses me slightly but I think it's a shortly way of doing exactly what I've done? so like function in function? - I'll take into account the mkdir -p that's very useful thankyou very much — DannyTheDev
– DannyTheDev, Commented Dec 14, 2014 at 20:24

Community · Accepted Answer · 2017-04-13 12:40:52Z

I'd say a bit stronger than @Etan Reisner: you should use $(...) instead of `...`. It's the modern, recommended way, and it makes nesting a lot easier and clearer.

But in the case of this code:

  path=`dirname $f`
  path=`dirname $path`

  # get the current log filename
  filename=`basename $f`

There's a slightly better way:

  path=${f%/*}
  path=${path%/*}

  # get the current log filename
  filename=${f##*/}

It's slightly better because it gives the same output but without spawning sub-processes. I do prefer using dirname and basename because they are clear about what they do, and that's important. So I recommend to stick with it. (Etan's version with the nested call: path=$(dirname "$(dirname "$f")"))

In case of short if statements like this:

  if [ ! -d "$path/stats" ]; then
    mkdir "$path/stats"
  fi

I suggest this more compact form:

  [ -d "$path/stats" ] || mkdir "$path/stats"

This is not great:

  prefix=(${filename//./ })

prefix here will be a Bash array, and as such the name "prefix" is misleading, since it doesn't sound like an array. And for the purpose that you use later in the code, you don't actually need an array. And finally, the base name of a file without the extension is not called a prefix. "basename" would be more suitable. And incidentally, the basename command can actually give you this exactly:

basename=$(basename "$f" .log)

This only works if the path ends with .log, but since that's the pattern you're using, it will work.

If you want to strip any kind of extension, then you could use the similar technique I showed above with pattern substitution:

basename=${filename%.*}

This is not great:

 if [ $prefix == 'access' ]; then
   prefix=''
 fi

Because, you don't need any of those quotes, and actually it would be better to double-quote $prefix. Using the compact form as earlier, the statement can be written simpler:

[ "$prefix" = access ] && prefix=

Thank you very much for explaining how I could improve parts of my code, I will definitely remember the short if statement, that will come in very handy — DannyTheDev
– DannyTheDev, Commented Dec 14, 2014 at 20:22
Actually the array usage was rather clever and very clear (once I understood what it was doing). Using expansion tricks to avoid dirname and basename works but only if you are sure you aren't going to hit one of the corner cases (see here for more on those). The compact form of an if is better but has the side-effect of returning "false" when it doesn't match where the if returns true in that case. So keep that in mind for makefiles and if you choose to use set -e (though you probably shouldn't). — Etan Reisner
– Etan Reisner, Commented Dec 15, 2014 at 13:43

Stack Exchange Network

Loop through all virtualhost log files and run goaccess on each file

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Loop through all virtualhost log files and run goaccess on each file

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions