Update stats processing

sitescripts/stats/bin/pagegenerator.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2013 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 os, re, codecs, simplejson, time, itertools
 from datetime import date
 from sitescripts.utils import get_config, setupStderr, get_custom_template_environment, cached
 import sitescripts.stats.common as common
 from sitescripts.stats.countrycodes import countrycodes
 
-@cached(())

[Sebastian Noack, 2013/08/26 16:05:22]

You passed an empty tuple as timeout. Obviously you want to make the timeout
forever, but this isn't the right way to do it. It though works in at least
recent versions of CPython, but that is just luck, since the Python language
specs, don't specify that a tuple is always larger than an int/float, as far as
I know. So it might fail with other Python implementations or future versions of
CPython. However why don't you just use float('Inf')?

[Wladimir Palant, 2013/08/27 07:34:28]

This behavior is somewhat specified as described here:
http://stackoverflow.com/a/3270689/785541

Anyway, float("inf") was introduced in Python 2.6, this code is older however. I
followed the advice to use an empty tuple because I liked it better than using
an arbitrary number like 1e400. Fixed.

+@cached(float("inf"))
 def get_template_environment():
   return get_custom_template_environment({
     "monthname": lambda value: date(int(value[0:4]), int(value[4:]), 1).strftime("%b %Y"),
     "weekday": lambda value: ["Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday"][int(value)],
     "countryname": lambda value: countrycodes.get(value, "Unknown"),
     "sortfield": lambda value, field: (field["sort"] if "sort" in field else default_sort)(value),
     "maxhits": lambda items: max(itertools.chain((value["hits"] for key, value in items), [1])),
     "maxbandwidth": lambda items: max(itertools.chain((value["bandwidth"] for key, value in items), [1])),
     "sumhits": lambda items: max(sum(value["hits"] for key, value in items), 1),
     "sumbandwidth": lambda items: max(sum(value["bandwidth"] for key, value in items), 1),
     "isspecial": lambda name, field: field["isspecial"](name) if "isspecial" in field else False,
   })
 
-@cached(())
+@cached(float("inf"))
 def get_main_page_template():
   return get_template_environment().get_template(get_config().get("stats", "mainPageTemplate"))
 
-@cached(())
+@cached(float("inf"))
 def get_file_stats_template():
   return get_template_environment().get_template(get_config().get("stats", "filePageTemplate"))
 
-@cached(())
+@cached(float("inf"))
 def get_file_overview_template():
   return get_template_environment().get_template(get_config().get("stats", "fileOverviewTemplate"))
 
 def default_sort(obj):
   return sorted(obj.items(), key=lambda (k,v): v["hits"], reverse=True)
 
 def ensure_dir(path):

[Sebastian Noack, 2013/08/26 16:05:22]

Instead of checking if the dir exist and creating it if not, I would rather just
create it and catch the OSError. This will be faster since the FS must be
queried only once and you get around the theoretical condition that the dir
doesn't exist when you check for its presence but suddenly was created by
somebody else when actually try to create it a (few) ms later.

   dir = os.path.dirname(path)
-  if not os.path.exists(dir):
+  try:
     os.makedirs(dir)

[Wladimir Palant, 2013/08/27 07:34:28]

Fixed. However, this is a common pattern in our code base and will have to be
fixed consistently everywhere.

+  except OSError:
+    pass
 
 def generate_main_page(outputfile, month, url, data):
   ensure_dir(outputfile)
   get_main_page_template().stream({
     "now": time.time(),
     "month": month,
     "url": url,
     "data": data,
   }).dump(outputfile)
 
@@ skipping 39 matching lines @@
           data = simplejson.load(file)
 
         overview_url = "../../overview-" + common.filename_encode(filename + ".html")
         filtered_urls = {}
         for field in common.fields:
           if field["name"] not in data:
             continue
           # Create filtered views for the first thirty values of a field if they
           # have filtered data.
           for name, value in get_template_environment().filters["sortfield"](data[field["name"]], field)[0:30]:
-            if filter(lambda k: k not in ("hits", "bandwidth"), value.keys()):

[Sebastian Noack, 2013/08/26 16:05:22]

No need to create a new list with the keys first. Just call filter(..., value).
If value is a dict, it is iterable, and produces its keys on iteration. Also you
might want to use itertools.ifilter instead of filter, which doesn't returns a
pre-computed list, but a generator that is evaluated on the fly.

[Wladimir Palant, 2013/08/27 07:34:28]

I rather use value.iterkeys() here - it's obvious what's meant then, with Python
dictionaries I'm frequently confused what I am iterating on.

As to itertools.ifilter(), I would rather not complicate the code here since
this spot isn't performance-critical.

[Wladimir Palant, 2013/08/27 11:59:47]

Reply by Sebastian:

It does the same, but isn't actually necessary, because the dict object itself
is iterable and will produce it keys on iteration.

[Wladimir Palant, 2013/08/27 12:42:01]

See my reply - I already know that. But I prefer it to be obvious that we are
iterating over keys and not items here.

+            if filter(lambda k: k not in ("hits", "bandwidth"), value.iterkeys()):
               outputfile = os.path.join(outputdir,
                   common.filename_encode(server_type),
                   common.filename_encode(month),
                   common.filename_encode(filename),
                   "filtered-%s-%s.html" % (
                     common.filename_encode(field["name"]),
                     common.filename_encode(name),
                   ))
               generate_file_stats(outputfile, month, baseURL + filename, overview_url,
                   value, filter={"field": field, "value": name})
 
               if not field["name"] in filtered_urls:
                 filtered_urls[field["name"]] = {}
-              filtered_urls[field["name"]][name] = outputfile
+              filtered_urls[field["name"]][name] = os.path.basename(outputfile)
 
         outputfile = os.path.join(outputdir,
             common.filename_encode(server_type),
             common.filename_encode(month),
             common.filename_encode(filename),
             "index.html")
         generate_file_stats(outputfile, month, baseURL + filename, overview_url,
             data, filtered_urls=filtered_urls)
 
         if filename not in filedata:
           filedata[filename] = {}
-        month_url = (common.filename_encode(month) + "/" +

[Sebastian Noack, 2013/08/26 16:05:22]

You should use os.path.join() here as well.

[Wladimir Palant, 2013/08/27 07:34:28]

No, definitely not going to use os.path.join() for URLs - this would produce
wrong URLs on Windows for example. And urlparse.urljoin() won't make the logic
here more obvious.

[Wladimir Palant, 2013/08/27 11:59:47]

Reply by Sebastian:

Oh, didn't noted, it was an url. In that case hardcoded slashes are ok, of
course. But I would rather use a format string here, with all the literal parts
already included, instead the plus operator. This is Python, not JS. ;) I only
use the plus operator to concatenate two strings. If you have more strings to
concatenate into a single string, it is more readable and faster to either use
format strings or str.join().

-                     common.filename_encode(filename) + "/" +
-                     "index.html")
+        month_url = "%s/%s/%s" % (common.filename_encode(month),
+            common.filename_encode(filename),
+            "index.html")
         filedata[filename][month] = {"url": month_url, "hits": data["hits"], "bandwidth": data["bandwidth"]}
 
     monthdata = {}
     for filename, data in filedata.iteritems():
       outputfile = os.path.join(outputdir,
           common.filename_encode(server_type),
           "overview-" + common.filename_encode(filename + ".html"))
       generate_file_overview(outputfile, baseURL + filename, data)
 
       if current_month in data:
         monthdata[filename] = dict(data[current_month])
 
     outputfile = os.path.join(outputdir, common.filename_encode(server_type), "index.html")
     generate_main_page(outputfile, current_month, baseURL, monthdata)
 
 if __name__ == '__main__':
   setupStderr()
 
   datadir = get_config().get("stats", "dataDirectory")
   outputdir = get_config().get("stats", "outputDirectory")
   generate_pages(datadir, outputdir)