Merge lp:~lucio.torre/txstatsd/distinct-plugin-fix-http into lp:~txstatsd-dev/txstatsd/distinct-plugin

[Lucio Torre]

1- makes it compatible with pg9.1 (the one in P)
2- fixes setup.py and friends, its now installable
3- fixes resource http argument parsing so we get an int out of the query args
4- fixes the get_top query so we get the correct number of hits per user
5- returns http:500 on errors

1- makes it compatible with pg9.1 (the one in P)
2- fixes setup.py and friends, its now installable
3- fixes resource http argument parsing so we get an int out of the query args
4- fixes the get_top query so we get the correct number of hits per user
5- returns http:500 on errors

Sorry about doing big integration tests, but it helped a lot to find the issues.

[Sidnei da Silva (sidnei)]

Looks good.

review: Approve

[Ubuntu One Server Tarmac Bot (ubuntuone-server-tarmac)]

The attempt to merge lp:~lucio.torre/txstatsd/distinct-plugin-fix-http into lp:~txstatsd-dev/txstatsd/distinct-plugin failed. Below is the output from the failed tests.

./bin/start-database.sh
## Starting postgres in /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1 ##
The files belonging to this database system will be owned by user "tarmac".
This user must also own the server process.

The database cluster will be initialized with locale C.
The default text search configuration will be set to "english".

fixing permissions on existing directory /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 28MB
creating configuration files ... ok
creating template1 database in /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data/base/1 ... ok
initializing pg_authid ... ok
initializing dependencies ... ok
creating system views ... ok
loading system objects' descriptions ... ok
creating conversions ... ok
creating dictionaries ... ok
setting privileges on built-in objects ... ok
creating information schema ... ok
vacuuming database template1 ... ok
copying template1 to template0 ... ok
copying template1 to postgres ... ok

Success. You can now start the database server using:

    /usr/lib/postgresql/8.4/bin/postgres -D /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data
or
    /usr/lib/postgresql/8.4/bin/pg_ctl -D /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data -l logfile start

waiting for server to start.... done
server started
CREATE ROLE
To set your environment so psql will connect to this DB instance type:
    export PGHOST=/mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1
## Done. ##
psql -h `pwd`/tmp/db1 -d distinct < bin/schema.sql
CREATE TABLE
CREATE TABLE
CREATE INDEX
./bin/start-redis.sh

WARNING: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the -A option the
next time you run initdb.
ERROR: role "client" already exists
NOTICE: CREATE TABLE will create implicit sequence "paths_id_seq" for serial column "paths.id"
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "paths_pkey" for table "paths"
NOTICE: CREATE TABLE / UNIQUE will create implicit index "paths_path_key" for table "paths"
/sbin/start-stop-daemon: stat /usr/bin/redis-server: No such file or directory (No such file or directory)
make: *** [start-redis] Error 2

[Ubuntu One Server Tarmac Bot (ubuntuone-server-tarmac)]

The attempt to merge lp:~lucio.torre/txstatsd/distinct-plugin-fix-http into lp:~txstatsd-dev/txstatsd/distinct-plugin failed. Below is the output from the failed tests.

./bin/start-database.sh
## Starting postgres in /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1 ##
The files belonging to this database system will be owned by user "tarmac".
This user must also own the server process.

The database cluster will be initialized with locale C.
The default text search configuration will be set to "english".

fixing permissions on existing directory /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 28MB
creating configuration files ... ok
creating template1 database in /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data/base/1 ... ok
initializing pg_authid ... ok
initializing dependencies ... ok
creating system views ... ok
loading system objects' descriptions ... ok
creating conversions ... ok
creating dictionaries ... ok
setting privileges on built-in objects ... ok
creating information schema ... ok
vacuuming database template1 ... ok
copying template1 to template0 ... ok
copying template1 to postgres ... ok

Success. You can now start the database server using:

    /usr/lib/postgresql/8.4/bin/postgres -D /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data
or
    /usr/lib/postgresql/8.4/bin/pg_ctl -D /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data -l logfile start

waiting for server to start.... done
server started
CREATE ROLE
To set your environment so psql will connect to this DB instance type:
    export PGHOST=/mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1
## Done. ##
psql -h `pwd`/tmp/db1 -d distinct < bin/schema.sql
CREATE TABLE
CREATE TABLE
CREATE INDEX
./bin/start-redis.sh
trial distinctdb/
                                                                        [ERROR]

===============================================================================
[ERROR]: distinctdb.tests.test_distinct

Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/twisted/trial/runner.py", line 563, in loadPackage
    module = modinfo.load()
  File "/usr/lib/python2.6/dist-packages/twisted/python/modules.py", line 381, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "/usr/lib/python2.6/dist-packages/twisted/python/reflect.py", line 464, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "/mnt/tarmac/cache/txstatsd/distinct-plugin/distinctdb/tests/test_distinct.py", line 25, in <module>
    from twisted.plugins import distinctdbplugin
  File "/mnt/tarmac/cache/txstatsd/distinct-plugin/twisted/plugins/distinctdbplugin.py", line 4, in <module>
    from txstatsd.itxstatsd import IMetricFactory
exceptions.ImportError: No module named txstatsd.itxstatsd
-------------------------------------------------------------------------------

FAILED (errors=1)

WARNING: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the -A option the
next time you run initdb.
ERROR: role "client" already exists
NOT...

[Ubuntu One Server Tarmac Bot (ubuntuone-server-tarmac)]

The attempt to merge lp:~lucio.torre/txstatsd/distinct-plugin-fix-http into lp:~txstatsd-dev/txstatsd/distinct-plugin failed. Below is the output from the failed tests.

./bin/start-database.sh
## Starting postgres in /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1 ##
The files belonging to this database system will be owned by user "tarmac".
This user must also own the server process.

The database cluster will be initialized with locale C.
The default text search configuration will be set to "english".

fixing permissions on existing directory /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 28MB
creating configuration files ... ok
creating template1 database in /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data/base/1 ... ok
initializing pg_authid ... ok
initializing dependencies ... ok
creating system views ... ok
loading system objects' descriptions ... ok
creating conversions ... ok
creating dictionaries ... ok
setting privileges on built-in objects ... ok
creating information schema ... ok
vacuuming database template1 ... ok
copying template1 to template0 ... ok
copying template1 to postgres ... ok

Success. You can now start the database server using:

    /usr/lib/postgresql/8.4/bin/postgres -D /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data
or
    /usr/lib/postgresql/8.4/bin/pg_ctl -D /mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1/data -l logfile start

waiting for server to start.... done
server started
CREATE ROLE
To set your environment so psql will connect to this DB instance type:
    export PGHOST=/mnt/tarmac/cache/txstatsd/distinct-plugin/tmp/db1
## Done. ##
psql -h `pwd`/tmp/db1 -d distinct < bin/schema.sql
CREATE TABLE
CREATE TABLE
CREATE INDEX
./bin/start-redis.sh
trial distinctdb/
distinctdb.tests.test_distinct
  TestDatabaseMetricStorage
    test_connect ... [OK]
    test_create_metric_id ... [OK]
    test_find_saved_data ... [OK]
    test_get_distinct_count ... [OK]
    test_get_distinct_top_value ... [OK]
    test_load_metric_id ... [OK]
    test_save_bucket_twice ... [OK]
  TestDistinctMetricReporter
    test_configure ... [OK]
    test_get_bucket_no ... [OK]
    test_max ... [OK]
    test_reports ... [OK]
  TestDistinctResource
    test_render_count_resource ... [OK]
    test_render_top_resource ... [OK]
  TestDistinctResourceIntegration
    test_count ... [OK]
    test_top ... ...

=== modified file 'bin/start-database.sh'
--- bin/start-database.sh	2011-12-08 21:08:38 +0000
+++ bin/start-database.sh	2012-04-26 18:22:25 +0000
@@ -23,6 +23,8 @@
         export PGBINDIR=/usr/lib/postgresql/8.4/bin
     elif [ -d /usr/lib/postgresql/8.3 ]; then
         export PGBINDIR=/usr/lib/postgresql/8.3/bin
+    elif [ -d /usr/lib/postgresql/9.1 ]; then
+        export PGBINDIR=/usr/lib/postgresql/9.1/bin
     else
         echo "Cannot find valid parent for PGBINDIR"
     fi
@@ -34,7 +36,6 @@
     (
         cat <<EOF
 search_path='\$user,public,ts2'
-add_missing_from=false
 log_statement='all'
 log_line_prefix='[%m] %q%u@%d %c '
 fsync = off
 
=== modified file 'bin/stop-database.sh'
--- bin/stop-database.sh	2011-12-08 21:08:38 +0000
+++ bin/stop-database.sh	2012-04-26 18:22:25 +0000
@@ -10,6 +10,8 @@
     PGBINDIR=/usr/lib/postgresql/8.4/bin
 elif [ -d /usr/lib/postgresql/8.3 ]; then
     PGBINDIR=/usr/lib/postgresql/8.3/bin
+elif [ -d /usr/lib/postgresql/9.1 ]; then
+    PGBINDIR=/usr/lib/postgresql/9.1/bin
 else
     echo "Cannot find valid parent for PGBINDIR"
 fi
 
=== modified file 'distinctdb/distinctmetric.py'
--- distinctdb/distinctmetric.py	2012-03-15 00:32:23 +0000
+++ distinctdb/distinctmetric.py	2012-04-26 18:22:25 +0000
@@ -26,6 +26,7 @@
 
     def _render_json_error(self, request, result):
         """Fetch the data, make the request return"""
+        request.setResponseCode(500)
         request.write(json.dumps({"error": result.getErrorMessage()}))
         request.finish()
 
@@ -36,7 +37,9 @@
 
     def render_GET(self, request):
         """Return the web request asynchronously."""
-        d = threads.deferToThread(getattr(self.target, self.name), **request.args)
+        d = threads.deferToThread(
+            getattr(self.target, self.name),
+            **dict((k, float(v[0])) for k, v in request.args.items()))
         d.addCallback(partial(self._render_json_result, request))
         d.addErrback(partial(self._render_json_error, request))
         return server.NOT_DONE_YET
@@ -48,8 +51,10 @@
     def __init__(self, reporter):
         resource.Resource.__init__(self)
         self.reporter = reporter
-        self.putChild("top", JSONMethodResource(self.reporter, "get_distinct_top_value"))
-        self.putChild("count", JSONMethodResource(self.reporter, "get_distinct_count"))
+        self.putChild("top",
+            JSONMethodResource(self.reporter, "get_distinct_top_value"))
+        self.putChild("count",
+            JSONMethodResource(self.reporter, "get_distinct_count"))
 
 
 class DistinctMetricReporter(object):
@@ -216,11 +221,10 @@
         path = self.prefix + self.name
         c = psycopg2.connect(self.dsn)
         cr = c.cursor()
-        cr.execute("SELECT value, COUNT(value) AS cnt FROM points "
+        cr.execute("SELECT value, SUM(count) AS cnt FROM points "
                    "INNER JOIN paths ON (paths.id = points.path_id) "
                    "WHERE paths.path = %s AND bucket BETWEEN %s AND %s"
                    "GROUP BY value ORDER BY cnt DESC LIMIT %s", (
                        path, since_bucket, until_bucket, how_many,))
         rows = cr.fetchall()
         return rows
-
 
=== modified file 'distinctdb/tests/test_distinct.py'
--- distinctdb/tests/test_distinct.py	2012-03-15 00:32:23 +0000
+++ distinctdb/tests/test_distinct.py	2012-04-26 18:22:25 +0000
@@ -20,11 +20,15 @@
 import redis
 
 from twisted.trial.unittest import TestCase
-from twisted.internet import reactor
+from twisted.internet import reactor, protocol, defer
 from twisted.plugin import getPlugins
 from twisted.plugins import distinctdbplugin
 from twisted.web.test.test_web import DummyRequest
 
+from twisted.application import internet
+from twisted.web import server
+from twisted.web.client import Agent
+
 from txstatsd.itxstatsd import IMetricFactory
 from txstatsd import service
 
@@ -46,7 +50,7 @@
 
     def get_distinct_top_value(self, since, until, how_many=20):
         self.called.append(("get_distinct_top_value", (since, until, how_many)))
-        return [("one", 1), ("two", 1)]    
+        return [("one", 1), ("two", 1)]
 
 
 class TestDistinctMetricReporter(TestCase):
@@ -130,6 +134,7 @@
         reporter = DummyReporter()
         request = DummyRequest([])
         resource = distinct.JSONMethodResource(reporter, "get_foo")
+
         def check(result):
             self.assertEquals({"result": "foo"},
                               json.loads("".join(request.written)))
@@ -145,9 +150,11 @@
     def test_render_top_resource(self):
         reporter = DummyReporter()
         request = DummyRequest([])
-        request.args = {"since": time.time(), "until": time.time() + 1}
+        request.args = {"since": [str(time.time())],
+                        "until": [str(time.time() + 1)]}
         resource = distinct.DistinctResource(reporter)
         child_resource = resource.getChildWithDefault("top", request)
+
         def check(result):
             self.assertEquals(json.dumps({"result": [("one", 1), ("two", 1)]}),
                               "".join(request.written))
@@ -159,9 +166,11 @@
     def test_render_count_resource(self):
         reporter = DummyReporter()
         request = DummyRequest([])
-        request.args = {"since": time.time(), "until": time.time() + 1}
+        request.args = {"since": [str(time.time())],
+                        "until": [str(time.time() + 1)]}
         resource = distinct.DistinctResource(reporter)
         child_resource = resource.getChildWithDefault("count", request)
+
         def check(result):
             self.assertEquals(json.dumps({"result": 42}),
                               "".join(request.written))
@@ -200,6 +209,107 @@
         dmr._save_bucket(dmr.bucket, bucket_no)
 
 
+class ResponseCollector(protocol.Protocol):
+
+    def __init__(self, finished):
+        self.finished = finished
+        self.data = []
+
+    def dataReceived(self, bytes):
+        self.data.append(bytes)
+
+    def connectionLost(self, reason):
+        self.finished.callback("".join(self.data))
+
+
+def collect_response(response):
+    d = defer.Deferred()
+    c = ResponseCollector(d)
+    response.deliverBody(c)
+    return d
+
+
+class ErrorDummyReporter(object):
+
+    def get_distinct_count(self, *args, **kweargs):
+        raise Exception("die!")
+
+
+class TestDistinctResourceIntegrationError(TestCase):
+
+    def setUp(self):
+        dmr = ErrorDummyReporter()
+        root = distinct.DistinctResource(dmr)
+        site = server.Site(root)
+        self.port = 12341
+        self.service = internet.TCPServer(self.port, site)
+        self.service.startService()
+
+    @defer.inlineCallbacks
+    def test_count(self):
+        agent = Agent(reactor)
+
+        result = yield agent.request('GET',
+            'http://localhost:%s/count?since=0&until=1000000'
+            % (self.port,))
+        self.assertEquals(500, result.code)
+
+    def tearDown(self):
+        self.service.stopService()
+
+
+class TestDistinctResourceIntegration(TestDatabase):
+
+    def setUp(self):
+        TestDatabase.setUp(self)
+        dmr = distinct.DistinctMetricReporter("test", dsn=self.dsn)
+        root = dmr.getResource()
+        site = server.Site(root)
+        self.port = 12341
+        self.service = internet.TCPServer(self.port, site)
+        self.service.startService()
+
+    def setup_bucket(self):
+        dmr = distinct.DistinctMetricReporter("test", dsn=self.dsn)
+        dmr.update("one")
+        dmr.update("two")
+        dmr._save_bucket(dmr.bucket, 1)
+        dmr.bucket = {}
+        dmr.update("one")
+        dmr.update("one")
+        dmr._save_bucket(dmr.bucket, 2)
+
+    @defer.inlineCallbacks
+    def test_count(self):
+        self.setup_bucket()
+
+        agent = Agent(reactor)
+
+        result = yield agent.request('GET',
+            'http://localhost:%s/count?since=0&until=1000000'
+            % (self.port,))
+        self.assertEquals(200, result.code)
+        data = yield collect_response(result)
+        self.assertEquals({"result": 2}, json.loads(data))
+
+    @defer.inlineCallbacks
+    def test_top(self):
+        self.setup_bucket()
+
+        agent = Agent(reactor)
+
+        result = yield agent.request('GET',
+            'http://localhost:%s/top?since=0&until=1000000&how_many=1'
+            % (self.port,))
+        self.assertEquals(200, result.code)
+        data = yield collect_response(result)
+        self.assertEquals({"result": [['one', 3]]}, json.loads(data))
+
+    def tearDown(self):
+        self.service.stopService()
+        TestDatabase.tearDown(self)
+
+
 class TestDatabaseMetricStorage(TestDatabase):
 
     def test_connect(self):
 
=== modified file 'distinctdb/version.py'
--- distinctdb/version.py	2011-12-08 21:07:55 +0000
+++ distinctdb/version.py	2012-04-26 18:22:25 +0000
@@ -1,1 +1,1 @@
-distinctplugin = "0.0.1"
+distinctdb = "0.0.1"
 
=== modified file 'setup.py'
--- setup.py	2011-12-08 21:14:57 +0000
+++ setup.py	2012-04-26 18:22:25 +0000
@@ -4,7 +4,7 @@
 
 from twisted.plugin import IPlugin, getPlugins
 
-from distinctplugin import version
+from distinctdb import version
 
 # If setuptools is present, use it to find_packages(), and also
 # declare our dependency on epsilon.
@@ -20,7 +20,7 @@
         Taken from storm setup.py.
         """
         packages = []
-        for directory, subdirectories, files in os.walk("distinctplugin"):
+        for directory, subdirectories, files in os.walk("distinctdb"):
             if '__init__.py' in files:
                 packages.append(directory.replace(os.sep, '.'))
         return packages
@@ -42,8 +42,8 @@
 
 setup(
     cmdclass={'install': TxPluginInstaller},
-    name="distinctplugin",
-    version=version.distinctplugin,
+    name="distinctdb",
+    version=version.distinctdb,
     description="A txstatsd plugin for distinct counts",
     author="txStatsD Developers",
     url="https://launchpad.net/txstatsd",