Issue 395 - Filter hits statistics backend

sitescripts/filterhits/bin/process_logs.py

+# coding: utf-8
+
+# This file is part of the Adblock Plus web scripts,
+# Copyright (C) 2006-2015 Eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+
+import MySQLdb, itertools, json, os, sys
+from sitescripts.utils import get_config
+import sitescripts.filterhits.common as common

[Sebastian Noack, 2015/03/02 10:04:01]

Nit: from ... import ...

[kzar, 2015/03/02 10:39:03]

Done.

+import sitescripts.filterhits.db as db
+import sitescripts.filterhits.geometrical_mean as geometrical_mean
+
+last_log_file = None
+
+def log_files(dir):
+  """
+  Provides a generator of filter hits log files for the given directory.
+  Works recursively, relative path of log file is returned.
+  """
+  for root, subdirs, files in os.walk(dir):
+    for f in files:
+      if os.path.splitext(f)[1] == ".log" and f[0].isdigit():
+        yield os.path.join(root, f)
+
+def read_data(log_file):
+  """
+  Read, parse and return the JSON data for the given log file name.
+  (As a side effect sets the global last_log_file to the log file name.)
+  """
+  global last_log_file
+  try:
+    with open(log_file, "r") as f:
+      # Skip past the date and GET parameters
+      current = last = ""
+      while last + current != '" ':
+        last, current = current, f.read(1)
+        if not current:
+          sys.exit("Unexpected EOF in log file %s" % log_file)
+
+      # Read the JSON
+      data = json.load(f)
+      # Keep track of the current log file in global variable in case we need to
+      # identify it later if there's a problem. (This works because the files are
+      # processed lazily.)
+      last_log_file = log_file
+  except IOError:
+    sys.exit("Could not read log file %s" % log_file)
+  return data
+
+if __name__ == "__main__":
+  if not len(sys.argv) == 2:
+    print "Usage: python -m sitescripts.filterhits.bin.process_logs /path/to/logs"
+    sys.exit(1)
+
+  interval = get_config().get("filterhitstats", "interval")
+
+  def read_update(f):
+    return geometrical_mean.update(interval, read_data(f))
+
+  if sys.argv[1].endswith(".log"):
+    sql = read_update(sys.argv[1])
+  else:
+    sql = itertools.chain.from_iterable(itertools.imap(read_update,
+                                                       log_files(sys.argv[1])))
+
+  try:
+    db_connection = db.connect()
+    db.write(db_connection, sql)
+  except (KeyError, MySQLdb.Error), e:
+    if isinstance(e, KeyError):

[Sebastian Noack, 2015/03/02 10:04:01]

Where does the KeyError come from? Certainly not form the code reading the
configuration since, you use dict.get there.

[kzar, 2015/03/02 10:39:03]

KeyError comes from processing invalid data, this is required because we no
longer validate the data. (Well we check they submitted a JSON object at least
but that's it!)

+      message = "KeyError (%s)" % str(e.args[0])
+    elif isinstance(e, MySQLdb.Error):
+      message = "MySQL error (%d) \"%s\"" % (e.args[0], e.args[1])
+    sys.exit("Failed to process file %s, all changes rolled back. %s\n" % (
+      last_log_file, message
+    ))
+  finally:
+    if db_connection:

[Sebastian Noack, 2015/03/02 10:04:01]

Again, if db.connect() fails, this results into a NameError. You need following
logic:

db_connection = db.connect()
try:
  try:
    ...
  except:
    ...
finally:
  db_connection.close()
  

[kzar, 2015/03/02 10:39:03]

Done.

[Sebastian Noack, 2015/03/02 10:56:36]

Sorry, I meant:

try:
  db_connection = db.connect()
  try:
    ...
  finally:
    db_connection.close()
except:
  ...

Otherwise, you ignore MySQL errors that occur while connecting.

[kzar, 2015/03/02 11:00:52]

Yea I realised that but I figure it's OK. This script is called from
the command line to process logs, if the database connection fails I
think the script probably should return an exception.

[Sebastian Noack, 2015/03/02 11:06:05]

The exception would be printed as well if handled by the code above. With your
line of arguing there would be no reason to handle any exceptions here. Please
make it consistent.

[kzar, 2015/03/02 11:18:50]

Well my line of reasoning was that if the database is down and we can't connect
any message that makes it clear that the database is down is sufficient -
including the exception itself.

With the processing exceptions however this is not the case because the user
needs to know which log file caused the problem - not just that a MySQL
exception / KeyError was thrown.

Either way it's not a big deal, I now catch the MySQL exception on connecting
and display a friendlier message instead.

+      db_connection.close()