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

[Wladimir Palant, 2015/03/26 22:56:50]

Nit: PEP 8 requires one import per line and a blank line between groups of
imports (https://www.python.org/dev/peps/pep-0008/#imports). Same in the other
files of course. Yes, I know that most existing scripts aren't following these
requirements yet.

[kzar, 2015/03/27 11:59:57]

Done.

[Sebastian Noack, 2015/03/27 13:12:19]

While we are on it, please also add a new line between corelib and own module
imports.

[kzar, 2015/03/27 15:10:50]

Done.

+from sitescripts.utils import get_config
+from sitescripts.filterhits import common, db, geometrical_mean

[Wladimir Palant, 2015/03/26 22:56:50]

common isn't actually used here.

[kzar, 2015/03/27 11:59:57]

Done.

+
+last_log_file = None

[Wladimir Palant, 2015/03/26 22:56:50]

Nit: IMHO this isn't meant to be exported, call it _last_log_file?

[kzar, 2015/03/27 11:59:57]

Done.

+
+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():

[Wladimir Palant, 2015/03/26 22:56:50]

Why the requirement that the first letter is a digit? You don't have that
requirement when validating command line parameters. IMHO it serves no purpose,
other than breaking this code should you change the file name format elsewhere.

[kzar, 2015/03/27 11:59:57]

That is a simple way to skip over other log files in the directory, namely
processing-errors.log. Not strictly necessary but it makes the tool easier to
use in practice.

[Wladimir Palant, 2015/03/27 16:29:06]

Aren't all logs we are interested in inside of subdirectories? |if root != dir
and os.path.splitext(f)[1] == ".log"|?

[kzar, 2015/03/27 22:15:00]

Not always, for example you might choose to only re-process today's logs in
which case all the log files _would_ be in the root directory.

+        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)

[Wladimir Palant, 2015/03/26 22:56:50]

Wouldn't that be a lot simpler if the date and the GET parameters were stored in
the first line of the file and the JSON data in the second? You would merely
need to read a line then...

[kzar, 2015/03/27 11:59:57]

Sure, I suppose this logic would be simpler that way. Originally it was done
this way as each log file contained many entries, one per line and I didn't
re-consider the format since we changed to having one entry per file.

Thing is this logic works well and has been well tested now I would prefer to
leave it alone. Otherwise I'll need to update the format for test data that
we've generated, the unit tests, the supporting tools for Kirill and test it all
still works.

[Sebastian Noack, 2015/03/27 13:12:19]

I agree with Wladimir FWIW. I also wouldn't consider any code tested, if it's
neither used by real users nor passed QA. But you make a point, if we are going
to change it, we should do it now. ;)

[kzar, 2015/03/27 15:10:50]

OK fair enough, Done.

[Wladimir Palant, 2015/03/27 16:29:06]

Yes, that's why we have reviews - so that we don't change this stuff *after* we
create lots of tools depending on it ;)

This should be changed now, it won't get any easier in a year.

[kzar, 2015/03/27 22:15:00]

Better yet, I changed it in the past :p. (See patch set 10)

+
+      # 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()
+  except MySQLdb.Error:
+    sys.exit("Failed to connect to the MySQL database.")

[Wladimir Palant, 2015/03/26 22:56:50]

I have the impression that the original exception will be more informative here,
why catch it?

[kzar, 2015/03/27 11:59:57]

This was Sebastian's idea, not mine. (See the inline discussion around here in
Patch set 5.)

[Sebastian Noack, 2015/03/27 13:12:19]

There are no comments on patchset 5. You probably mean this discussion:
http://codereview.adblockplus.org/4615801646612480/patch/5768755258851328/619...

I didn't suggest to print this string instead the original exception here. That
was your decision. I merely pointed out that you have to open the try/finally
block closing the connection after you actually create the connection.

[kzar, 2015/03/27 15:10:50]

OK the discussion was in patch set 4... it doesn't matter.

+
+  try:
+    db.write(db_connection, sql)
+  except (KeyError, MySQLdb.Error), e:
+    if isinstance(e, KeyError):
+      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:
+    db_connection.close()