Issue 239 - Move AWStats processing to the stats server

Issue 239 - Move AWStats processing to the stats server

[Wladimir Palant, 2014-04-24 10:01:04]

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/mani...
File manifests/intraforumserver.pp (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/mani...
manifests/intraforumserver.pp:2: include base, ssh, statsclient
Leaving ssh here was a mistake apparently, statsclient instantiates the ssh
class as well. Fixed in
https://hg.adblockplus.org/infrastructure/rev/c064658dd81b.

[Wladimir Palant, 2014-04-24 18:57:03]

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/files/known_hosts (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/files/known_hosts:4:
server_7.adblockplus.org,88.198.50.132,2a01:4f8:131:2144::2 ecdsa-sha2-nistp256
AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBDKYm9Ajuo3BOVhc/UCTzc76MHHqAs/szmj1APDyVMq6/7XROEEB6qQ1uuaijNQBBy7PylbG9tYbkUTNRrqt7mg=
server_10 is missing here, fixed that in
https://hg.adblockplus.org/infrastructure/rev/fc5c554de095

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/manifests/awstats.pp (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/manifests/awstats.pp:117: 'youtube.adblockplus.org' => {
The site is called youtube.adblockplus.me, not youtube.adblockplus.org, fixed
that in https://hg.adblockplus.org/infrastructure/rev/ea9e282d51b1

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/manifests/awstats.pp:130: host =>
'server10.adblockplus.org',
The DNS name is server_10, fixed that in
https://hg.adblockplus.org/infrastructure/rev/eee27e75236e

[Felix Dahlke, 2014-04-25 04:37:08]

Looks good, some comments.

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/files/site.conf (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/files/site.conf:40: autoindex on;   # for the archives
Micro nit: Single space before #?

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/manifests/awstats.pp (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/manifests/awstats.pp:4: file {'/var/www/awstatsdata':
How about combining these directory resources?

file {['/var/www/awstatsdata',
       '/var/www/awstatsconf',
       '/var/www/awstats',
       '/var/www/awstats/archive']:
  ensure => directory,
  owner => root,
  mode => 0755
}

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/manifests/awstats.pp:75: file
{["/var/www/awstatsdata/$title", "/var/www/awstats/$title",
"/var/www/awstats/archive/$title"]:
Nit: Wrap at 80 columns?

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/manifests/downloads.pp (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/manifests/downloads.pp:8: file {'/var/www/stats':
Could also be merged with the file resource below for brevity.

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/manifests/downloads.pp:35: environment =>
['MAILTO=admins@adblockplus.org,root', 'PYTHONPATH=/opt/sitescripts'],
Nit: Wrap at 80 columns? (Note that for the line above, wrapping it would
arguably hurt readability)

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/templates/awstats.conf (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/templates/awstats.conf:224: # AWStats saves and sorts its
database on a month basis (except if using
Does this mean we're storing full IPs?

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/templates/build_static_head.erb (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/templates/build_static_head.erb:1: #!/usr/bin/env python
Why is this a template? Seems to me like it could be a static file, assuming
concat can deal with a mixture of file and template fragments.

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/templates/build_static_head.erb:3: import subprocess, sys,
os, re
Should be one import per package, according to PEP8 [1]. However, we might want
to overrule that, since we're not overly consistent there so far IIRC.

(Note that the import line below is fine according to PEP8.)

[1] http://legacy.python.org/dev/peps/pep-0008/

[Wladimir Palant, 2014-04-25 07:40:55]

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/templates/awstats.conf (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/templates/awstats.conf:224: # AWStats saves and sorts its
database on a month basis (except if using
On 2014/04/25 04:37:08, Felix H. Dahlke wrote:
> Does this mean we're storing full IPs?

Yes, in the VISITOR section of the AWStats data. There doesn't seem to be a
simple solution to that, IMHO the easiest one is a script running along with
"build_static prevmonth" that will anonymize all IP addresses in the VISITOR
section of the previous month.

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/templates/build_static_head.erb (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/templates/build_static_head.erb:1: #!/usr/bin/env python
On 2014/04/25 04:37:08, Felix H. Dahlke wrote:
> Why is this a template? Seems to me like it could be a static file, assuming
> concat can deal with a mixture of file and template fragments.

Sure, it can be a static file - it's merely a template to have all pieces of the
file in one place. Same goes for the other head/foot templates.

[Felix Dahlke, 2014-04-25 09:42:50]

LGTM

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
File modules/statsmaster/templates/build_static_head.erb (right):

http://codereview.adblockplus.org/6293281981857792/diff/5629499534213120/modu...
modules/statsmaster/templates/build_static_head.erb:1: #!/usr/bin/env python
On 2014/04/25 07:40:55, Wladimir Palant wrote:
> On 2014/04/25 04:37:08, Felix H. Dahlke wrote:
> > Why is this a template? Seems to me like it could be a static file, assuming
> > concat can deal with a mixture of file and template fragments.
> 
> Sure, it can be a static file - it's merely a template to have all pieces of
the
> file in one place. Same goes for the other head/foot templates.

Fair enough.