Analyse an Nginx access log with awk and produce a report

Question

I'm looking for your opinions on anything that I am doing wrong in the application below, such as best practices, glaringly horrible errors or even just your own personal opinion.

Task: Grab an Nginx access log, ask it some questions and dump responses out to a templated report.

I have removed all comments from this post to keep the reading to a minimum. You can see the full version at GitHub.

#!/usr/bin/python
import datetime
from os import remove
from subprocess import Popen
from subprocess import PIPE
from subprocess import check_output
from string import Template
class NginxReporter():
 location = None
 template = 'templates/default.html'
 html = None
 def __init__(self, location=None, template=None, execute=False):
 self.location = location
 if template:
 self.template = template
 if execute:
 self.parse()
 self.output()
 self.delete()
 def scpdownload(self, remotelocation=None):
 if remotelocation:
 check_output(['scp', remotelocation, self.location])
 def delete(self):
 remove(self.location)
 def parse(self):
 html = {}
 codes = Popen("awk '{print 9ドル}' " + self.location +
 " | sort | uniq -c | sort -r", shell=True, stdout=PIPE)
 html['codes'] = self.build(codes)
 statuses = [206, 301, 302, 400, 404, 408, 499, 500]
 for status in statuses:
 statuslist = Popen("awk '(9ドル ~ /" + str(status) + "/)' "
 + self.location + " | awk '{print 7ドル}' " +
 " | sort | uniq -c | sort -r",
 shell=True, stdout=PIPE)
 html["http_"+str(status)+"_code"] = self.build(statuslist)
 html['generated'] = datetime.datetime.today().strftime('%Y-%m-%d %H:%M')
 self.html = html
 def build(self, output):
 block = ""
 while True:
 out = output.stdout.read()
 if out == '' and output.poll() != None:
 break
 if out != '':
 block = out
 return block
 def output(self):
 file = open(self.template)
 block = Template(file.read())
 print block.substitute(self.html)

Usage

report = NginxReporter('access.log')
""" report.scpdownload("[email protected]:/path/to/nginx/access.log") """
report.parse()
report.output()

Answer 1

The biggest problem, and the hardest to fix, is calling awk, sort | uniq -c | sort -r, sometimes within a loop, when you could implement all that more efficiently in Python. It's ugly to do this in multiple sub-shells and multiple processes. But that would take a substantial rewrite.

---

You are not using the class attributes here:

class NginxReporter():
 location = None
 template = 'templates/default.html'
 html = None

So you could just delete these from your class.

---

The constructor is not well designed. You're trying to do too much, for two very different use cases:

1. In one use case, you use it just to set a logfile location, and then you call other methods to do the real work
2. In another use case, the constructor would call the other methods

It's better to decide one of these ways, stick with it, and simplify the code accordingly.

---

This method is strange:

def scpdownload(self, remotelocation=None):
 if remotelocation:
 check_output(['scp', remotelocation, self.location])

You allow calling it without params, or null param, to do nothing? This is messy and confusing. It would be better to require a param.

Btw, downloading a file with scp doesn't sound like it belongs to the class. It violates the single responsibility principle: a class should have one clear purpose (parse nginx log files), and nothing else.

---

This could use some reworking:

if out == '' and output.poll() != None:
 break
if out != '':
 block = out

Like this:

if not out:
 if not output.poll():
 break
else:
 block = out

---

You open a file for reading but you're not closing it:

file = open(self.template)
block = Template(file.read())
print block.substitute(self.html)

The right way to do is using with:

with open(self.template) as fh:
 block = Template(fh.read())
 print block.substitute(self.html)

I also renamed file to fh because file is a class in Python 2.x

---

If you want to temporarily comment out one line, it's better to use # instead of """ ... """ which is recommended for multi-line comments and docstrings:

report = NginxReporter('access.log')
#report.scpdownload("[email protected]:/path/to/nginx/access.log")
report.parse()
report.output()

• Ne Ma: Fantastic! I mean really great feedback. I will modify code appropriately and update for anyone else who stumbles across this to chip in.
• janos: Glad you like it! For the record, you're not supposed to edit the code in your post as that would invalidate my answer ;-)
• Ne Ma: Updated code example. I am still confused by your point that the class variables are not being used. I have updated the code(tbh it did not even need a constructor with my default values) so perhaps that changes something.