Ema has submitted this change and it was merged. ( 
https://gerrit.wikimedia.org/r/326965 )

Change subject: varnishxcps: use varnishncsa to read log entries from the VSM
......................................................................


varnishxcps: use varnishncsa to read log entries from the VSM

varnsihlog.py and python-varnishapi are not performant enough to read
from the VSM without overruns. As an alternative approach, call
varnishncsa instead.

We define a base class called cachestats.CacheStatsSender responsible
for: i) abstracting away the statsd-related operation, which are common
to all our varnish python stats modules, and ii) calling an external
command (varnishncsa) to read the shared memory log. Another class
called XcpsCacheStatsSender is responsible for dealing with the
xcps-specific details of parsing X-Connection-Properties and generating
the stats.

Bug: T151643
Change-Id: Ied623dfbbd5004e129761287e2e566492a0ed1a7
---
A modules/varnish/files/cachestats.py
M modules/varnish/files/varnishxcps
M modules/varnish/manifests/common.pp
M modules/varnish/manifests/logging/xcps.pp
4 files changed, 151 insertions(+), 71 deletions(-)

Approvals:
  Ema: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/modules/varnish/files/cachestats.py 
b/modules/varnish/files/cachestats.py
new file mode 100644
index 0000000..fa3ee28
--- /dev/null
+++ b/modules/varnish/files/cachestats.py
@@ -0,0 +1,111 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+"""
+  cachestats
+  ~~~~~~~~~~
+  cachestats.CacheStatsSender is responsible for abstracting away any
+  statsd-related operation, common to all our varnish python stats modules, as
+  well as calling an external command (eg: varnishncsa) to read from VSM.
+  Subclasses are responsible for dealing with the details of parsing the log
+  entries and generating stats.
+
+  Copyright 2016 Emanuele Rocca <e...@wikimedia.org>
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+"""
+
+import argparse
+import io
+import os
+import socket
+import time
+import urlparse
+
+from subprocess import PIPE, Popen
+
+
+def parse_statsd_server_string(server_string):
+    """Convert statsd server string into (hostname, port) tuple."""
+    parsed = urlparse.urlparse('//' + server_string)
+    return parsed.hostname, parsed.port or 8125
+
+
+def parse_prefix_string(key_prefix):
+    key_prefix = key_prefix.strip('.')
+    if not key_prefix:
+        raise ValueError('Key prefix must not be empty')
+    return key_prefix
+
+
+class CacheStatsSender(object):
+
+    cmd = []
+    description = ''
+
+    def __init__(self, argument_list):
+        """Parse CLI arguments, initialize self.stats and statsd socket.
+
+        argument_list is a list such as ['--foo', 'FOO', '--bar', 'BAR']"""
+        ap = argparse.ArgumentParser(
+            description=self.description,
+            epilog='If no statsd server is specified, '
+                   'prints stats to stdout instead.')
+
+        ap.add_argument('--statsd-server', help='statsd server',
+                        type=parse_statsd_server_string, default=None)
+        ap.add_argument('--key-prefix', help='metric key prefix',
+                        type=parse_prefix_string, default='varnish.clients')
+        ap.add_argument('--interval', help='send interval',
+                        type=int, default=30)
+        self.args = ap.parse_args(argument_list)
+
+        self.next_pub = time.time() + self.args.interval
+
+        self.sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+        self.stats = {}
+
+    def gen_stats(self, record):
+        """Update the self.stats dictionary. Implementation left to the
+        subclasses"""
+        raise NotImplementedError()
+
+    def handle_record(self, record):
+        """Update the self.stats dictionary by calling self.gen_stats. Send
+        stats to statsd at regular intervals defined by self.args.interval"""
+        self.gen_stats(record)
+
+        now = time.time()
+        if now >= self.next_pub:
+            self.next_pub = now + self.args.interval
+            buf = io.BytesIO()
+            while self.stats:
+                metric = '%s:%s|c\n' % self.stats.popitem()
+                buf.write(metric.encode('utf-8'))
+            buf.seek(io.SEEK_SET)
+            if self.args.statsd_server:
+                self.sock.sendto(buf.read(), self.args.statsd_server)
+            else:
+                print(buf.read().decode('utf-8', errors='replace').rstrip())
+
+    def main(self):
+        """Execute the command specified in self.cmd and call handle_record for
+        each output line produced by the command"""
+        p = Popen(self.cmd, stdout=PIPE, bufsize=-1)
+
+        try:
+            while True:
+                line = p.stdout.readline()
+                self.handle_record(line)
+        except KeyboardInterrupt:
+            os.waitpid(p.pid, 0)
diff --git a/modules/varnish/files/varnishxcps 
b/modules/varnish/files/varnishxcps
index 13289da..c24feed 100755
--- a/modules/varnish/files/varnishxcps
+++ b/modules/varnish/files/varnishxcps
@@ -4,7 +4,7 @@
   varnishxcps
   ~~~~~~~~~~~
 
-  Accumulate X-Client-Connection stats and report them to StatsD.
+  Accumulate X-Connection-Properties stats and report them to StatsD.
 
   Usage: varnishxcps [--statsd-server SERVER] [--key-prefix PREFIX]
 
@@ -12,6 +12,7 @@
     --key-prefix PREFIX     metric key prefix (default: varnish.clients)
 
   Copyright 2015 Ori Livneh <o...@wikimedia.org>
+  Copyright 2016 Emanuele Rocca <e...@wikimedia.org>
 
   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
@@ -26,78 +27,38 @@
   limitations under the License.
 
 """
-import argparse
-import io
+
 import re
-import socket
-import threading
-import urlparse
-import time
+import sys
 
-import varnishlog
+from cachestats import CacheStatsSender
 
 
-def parse_statsd_server_string(server_string):
-    parsed = urlparse.urlparse('//' + server_string)
-    return parsed.hostname, parsed.port or 8125
+class XcpsCacheStatsSender(CacheStatsSender):
+
+    cmd = ['/usr/bin/varnishncsa', '-n', 'frontend',
+           '-q', 'ReqMethod ne "PURGE"',
+           '-F', '%{X-Connection-Properties}i']
+
+    description = 'X-Connection-Properties StatsD reporter'
+
+    def __init__(self, argument_list):
+        super(XcpsCacheStatsSender, self).__init__(argument_list)
+        self.key_value_pairs = re.compile('([A-Z][A-Z0-9]*)=([^;]+)')
+
+    def gen_stats(self, record):
+        for k, v in self.key_value_pairs.findall(record):
+            if k == 'SSR':
+                k = 'ssl_sessions'
+                v = 'reused' if v == '1' else 'negotiated'
+            elif k == 'C':
+                k = 'ssl_cipher'
+            elif k == 'EC':
+                k = 'ssl_ecdhe_curve'
+            v = v.replace('.', '_')
+            s = '.'.join((self.args.key_prefix, k, v)).lower()
+            self.stats[s] = self.stats.get(s, 0) + 1
 
 
-def parse_prefix_string(key_prefix):
-    key_prefix = key_prefix.strip('.')
-    if not key_prefix:
-        raise ValueError('Key prefix must not be empty')
-    return key_prefix
-
-
-ap = argparse.ArgumentParser(
-    description='X-Connection-Properties StatsD reporter',
-    epilog='If no statsd server is specified, prints stats to stdout instead.'
-)
-ap.add_argument('--statsd-server', help='statsd server',
-                type=parse_statsd_server_string, default=None)
-ap.add_argument('--key-prefix', help='metric key prefix',
-                type=parse_prefix_string, default='varnish.clients')
-ap.add_argument('--interval', help='send interval',
-                type=int, default=30)
-args = ap.parse_args()
-
-sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
-key_value_pairs = re.compile('([A-Z][A-Z0-9]*)=([^;]+)')
-stats = {}
-
-
-def vsl_callback(transaction_id, tag, record, remote_party):
-    for k, v in key_value_pairs.findall(record):
-        if k == 'SSR':
-            k = 'ssl_sessions'
-            v = 'reused' if v == '1' else 'negotiated'
-        elif k == 'C':
-            k = 'ssl_cipher'
-        elif k == 'EC':
-            k = 'ssl_ecdhe_curve'
-        v = v.replace('.', '_')
-        s = '.'.join((args.key_prefix, k, v)).lower()
-        stats[s] = stats.get(s, 0) + 1
-
-    now = time.time()
-    if now >= vsl_callback.next_pub:
-        vsl_callback.next_pub = now + args.interval
-        buf = io.BytesIO()
-        while stats:
-            metric = '%s:%s|c\n' % stats.popitem()
-            buf.write(metric.encode('utf-8'))
-        buf.seek(io.SEEK_SET)
-        if args.statsd_server:
-            sock.sendto(buf.read(), args.statsd_server)
-        else:
-            print(buf.read().decode('utf-8', errors='replace').rstrip())
-
-    return 0
-
-vsl_callback.next_pub = time.time() + args.interval
-
-varnishlog.varnishlog((
-    ('q', 'ReqMethod ne "PURGE"'),
-    ('I', 'ReqHeader:^X-Connection-Properties:'),
-    ('n', 'frontend'),
-), vsl_callback)
+if __name__ == "__main__":
+    XcpsCacheStatsSender(sys.argv[1:]).main()
diff --git a/modules/varnish/manifests/common.pp 
b/modules/varnish/manifests/common.pp
index 033a76c..bdf6745 100644
--- a/modules/varnish/manifests/common.pp
+++ b/modules/varnish/manifests/common.pp
@@ -104,4 +104,12 @@
         mode    => '0444',
         require => 
File['/usr/local/lib/python2.7/dist-packages/varnishapi.py'],
     }
+
+    # Install cachestats.py
+    file { '/usr/local/lib/python2.7/dist-packages/cachestats.py':
+        source => 'puppet:///modules/varnish/cachestats.py',
+        owner  => 'root',
+        group  => 'root',
+        mode   => '0444',
+    }
 }
diff --git a/modules/varnish/manifests/logging/xcps.pp 
b/modules/varnish/manifests/logging/xcps.pp
index 6822c6d..dc6e166 100644
--- a/modules/varnish/manifests/logging/xcps.pp
+++ b/modules/varnish/manifests/logging/xcps.pp
@@ -23,7 +23,7 @@
         owner   => 'root',
         group   => 'root',
         mode    => '0555',
-        require => 
File['/usr/local/lib/python2.7/dist-packages/varnishlog.py'],
+        require => 
File['/usr/local/lib/python2.7/dist-packages/cachestats.py'],
         notify  => Service['varnishxcps'],
     }
 

-- 
To view, visit https://gerrit.wikimedia.org/r/326965
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ied623dfbbd5004e129761287e2e566492a0ed1a7
Gerrit-PatchSet: 7
Gerrit-Project: operations/puppet
Gerrit-Branch: production
Gerrit-Owner: Ema <e...@wikimedia.org>
Gerrit-Reviewer: BBlack <bbl...@wikimedia.org>
Gerrit-Reviewer: Elukey <ltosc...@wikimedia.org>
Gerrit-Reviewer: Ema <e...@wikimedia.org>
Gerrit-Reviewer: Volans <rcocci...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to