Pull request for influxdb #482

+    @staticmethod
+    @tenacity.retry(retry=tenacity.retry_if_exception_type(
+        (indexer.NoSuchResource, indexer.NamedMetricAlreadyExists)))
+    def get_or_create_resource_and_metrics(resource_type, creator, rid,


Maybe we can share it with the one in Prometheus

Yeah I can try to do that now that Prometheus is merged.

sileht · 2017-11-23T06:28:20Z

gnocchi/tests/indexer/sqlalchemy/test_migrations.py

@@ -49,6 +49,20 @@ def setUp(self):
        self.index = indexer.get_driver(self.conf)
        self.index.upgrade(nocreate=True)
        self.addCleanup(self._drop_database)
+        # NOTE(sileht): remove tables dynamically created by other tests


That may need to be rebased, mine have been merged.

jd · 2017-11-23T09:48:05Z

Pull-request updated, HEAD is now 53989ae

ghost · 2017-11-23T10:43:04Z

Tests ERROR for HEAD 53989ae

TARGET=pep8: FAILED
TARGET=docs: FAILED
TARGET=docs-gnocchi.xyz: FAILED
TARGET=py27-mysql-ceph-upgrade-from-3.1: ERRORED
TARGET=py35-postgresql-file-upgrade-from-3.1: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.0: ERRORED
TARGET=py35-postgresql-file-upgrade-from-4.0: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.1: ERRORED
TARGET=py35-postgresql-file-upgrade-from-4.1: PASSED
TARGET=py27-mysql: FAILED
TARGET=py35-mysql: PASSED
TARGET=py27-postgresql: FAILED
TARGET=py35-postgresql: PASSED

jd · 2017-11-23T14:42:14Z

Pull-request updated, HEAD is now 439d1d3

jd · 2017-11-23T14:57:08Z

Pull-request updated, HEAD is now 439d1d3

ghost · 2017-11-23T15:19:42Z

Tests SUCCESS for HEAD 439d1d3

TARGET=pep8: PASSED
TARGET=docs: PASSED
TARGET=docs-gnocchi.xyz: PASSED
TARGET=py27-mysql-ceph-upgrade-from-3.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-3.1: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.0: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.0: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.1: PASSED
TARGET=py27-mysql: PASSED
TARGET=py35-mysql: PASSED
TARGET=py27-postgresql: PASSED
TARGET=py35-postgresql: PASSED

jd · 2017-11-27T13:35:52Z

Pull-request updated, HEAD is now aa33aee

chungg · 2017-11-27T14:20:38Z

gnocchi/rest/api.py

@@ -2055,6 +2057,7 @@ def __init__(self):
            "capabilities": CapabilityController(),
            "status": StatusController(),
            "aggregates": agg_api.AggregatesController(),
+            "influxdb": influxdb.InfluxDBController(),


can we enable this on demand?

Probably, though we don't do that for Prometheus either. So what would be the usecase?

we do, it's only enabled if the right libs are there although we can't do the same here... i just don't want to load it unless explicit.

If I have snappy installed on my system for whatever reason then I cannot disable Prometheus support.
This isn't a correct response. :/

That being said, we need something more than just a switch IMHO, see #499. So I'd postponed any switch until we solve #499.

chungg · 2017-11-27T14:26:18Z

gnocchi/rest/influxdb.py

+            except pyparsing.ParseException:
+                api.abort(501, {"cause": "Not implemented error",
+                                "detail": "q",
+                                "reason": "Querying is not implemented"})


this error is confusing... i don't see how 'querying is not implemented', this is literally what the endpoint is. is it something 'unsupported' that will throw this?

We don't support InfluxDB query language. I changed to "query not implemented" to be more precise about the fact that this query is not implemented.
The only supported is CREATE DATABASE that's not what I'd call a query ;)

chungg · 2017-11-27T14:31:17Z

gnocchi/rest/influxdb.py

+    def _write_get_lines():
+        encoding = pecan.request.headers.get('Transfer-Encoding', "").lower()
+        if encoding == "chunked":
+            if uwsgi is None:


how usable is this without chunked? if it isn't, i don't think we should support this especially since we're checking this constantly.

it's completely usable without chunked encoding. That's what the tests do.

But real InfluxDB client sends chunked encoding.

so unchunked is only for tests?

In real life, really likely, until someone starts to talk InfluxDB to Gnocchi in regular encoding (but I don't think InfluxDB does that?)

chungg · 2017-11-27T14:40:00Z

gnocchi/rest/influxdb.py

+                    # If this function raises ResourceAlreadyExists it means
+                    # the resource might already exist as another type, we
+                    # can't continue.
+                    LOG.error("Unable to get resource `%s' for InfluxDB, "


s/get/create/

chungg · 2017-11-27T14:44:19Z

gnocchi/tests/functional/gabbits/influxdb.yaml

+        $.creator: admin
+
+    - name: check metric created
+      GET: /v1/resource/influxdbtest/foobar/metric/mymetric.field@mytag=myvalue


this should assert something.

it does, it asserts there's no error being returned (i.e. no 404).

it does not check any content since it's not the purpose of this test. It's not testing the format, it's testing the metric creation.

word. didn't realise gabbi would auto fail for these cases. i'd prefer it be explicit and we validate more than just 'it didn't explode' but your call.

yeah it's always a trade-off: asserting 200 for example would break if we ever change the code to be 202 for example, whereas here we don't really care.

Though I agree in this case there's 0% chance we ever change from 200 :)

chungg · 2017-11-27T14:45:11Z

gnocchi/tests/functional/gabbits/influxdb.yaml

+        $.creator: admin
+
+    - name: check metric created different tag resource id
+      GET: /v1/resource/influxdbtest/myvalue/metric/mymetric.field@host=foobar


assert something

chungg · 2017-11-27T14:45:15Z

gnocchi/tests/functional/gabbits/influxdb.yaml

+      GET: /v1/resource/influxdbtest/myvalue/metric/mymetric.field@host=foobar
+
+    - name: check metric created different tag resource id and slash replaced
+      GET: /v1/resource/influxdbtest/myvalue/metric/cpu.field@path=_foobar


assert something

ghost · 2017-11-27T14:55:36Z

Tests SUCCESS for HEAD aa33aee

TARGET=pep8: PASSED
TARGET=docs: PASSED
TARGET=docs-gnocchi.xyz: PASSED
TARGET=py27-mysql-ceph-upgrade-from-3.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-3.1: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.0: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.0: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.1: PASSED
TARGET=py27-mysql: PASSED
TARGET=py35-mysql: PASSED
TARGET=py27-postgresql: PASSED
TARGET=py35-postgresql: PASSED

Fixed query

jd · 2017-11-27T15:54:22Z

Pull-request updated, HEAD is now 8b0b52f

ghost · 2017-11-27T16:17:55Z

Tests FAILURE for HEAD 8b0b52f

TARGET=pep8: PASSED
TARGET=docs: PASSED
TARGET=docs-gnocchi.xyz: PASSED
TARGET=py27-mysql-ceph-upgrade-from-3.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-3.1: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.0: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.0: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.1: PASSED
TARGET=py27-mysql: FAILED
TARGET=py35-mysql: FAILED
TARGET=py27-postgresql: FAILED
TARGET=py35-postgresql: FAILED

jd · 2017-11-27T17:05:17Z

Fix test after 'Query' word change

ghost · 2017-11-27T17:23:58Z

Tests FAILURE for HEAD 17ff947

TARGET=pep8: PASSED
TARGET=docs: PASSED
TARGET=docs-gnocchi.xyz: PASSED
TARGET=py27-mysql-ceph-upgrade-from-3.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-3.1: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.0: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.0: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.1: PASSED
TARGET=py27-mysql: FAILED
TARGET=py35-mysql: FAILED
TARGET=py27-postgresql: FAILED
TARGET=py35-postgresql: FAILED

Using --http enables the HTTP router – which is not used here. That router prevents the chunked tranfer encoding to work.

…n optional

…lass This is going to be used by other code.

…metrics

This timeout is going to be used as a more general timeout for operations that the API needs to do and that block for a long time.

This provides a new InfluxDB-compatible endpoint to write data to Gnocchi at `/v1/influxdb'.

jd · 2017-11-27T18:15:43Z

Pull-request updated, HEAD is now b4264ae

ghost · 2017-11-27T19:09:54Z

Tests SUCCESS for HEAD b4264ae

TARGET=pep8: PASSED
TARGET=docs: PASSED
TARGET=docs-gnocchi.xyz: PASSED
TARGET=py27-mysql-ceph-upgrade-from-3.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-3.1: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.0: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.0: PASSED
TARGET=py27-mysql-ceph-upgrade-from-4.1: PASSED
TARGET=py35-postgresql-file-upgrade-from-4.1: PASSED
TARGET=py27-mysql: PASSED
TARGET=py35-mysql: PASSED
TARGET=py27-postgresql: PASSED
TARGET=py35-postgresql: PASSED

sileht

You finally not sharing get_or_create_resource_and_metrics() with prometheus ?

jd · 2017-12-01T12:13:16Z

You finally not sharing get_or_create_resource_and_metrics() with prometheus ?

@sileht of course it is shared. did I miss something?

sileht · 2017-12-01T12:52:40Z

@jd, no it's OK I have misread

jd force-pushed the influxdb branch from 27fda5e to 0d8f2b7 Compare November 14, 2017 10:41

jd force-pushed the influxdb branch from 0d8f2b7 to 4f5e4d7 Compare November 15, 2017 10:51

jd force-pushed the influxdb branch from 4f5e4d7 to ad617be Compare November 16, 2017 09:30

jd force-pushed the influxdb branch from ad617be to 790ff33 Compare November 16, 2017 10:10

jd force-pushed the influxdb branch from 790ff33 to 1c6405f Compare November 16, 2017 11:16

jd force-pushed the influxdb branch from 1c6405f to 31a2b7b Compare November 16, 2017 13:46

sileht reviewed Nov 23, 2017

View reviewed changes

jd force-pushed the influxdb branch from 31a2b7b to 53989ae Compare November 23, 2017 09:48

jd force-pushed the influxdb branch from 53989ae to 439d1d3 Compare November 23, 2017 14:42

jd added api enhancement labels Nov 25, 2017

jd force-pushed the influxdb branch from 439d1d3 to aa33aee Compare November 27, 2017 13:35

chungg previously requested changes Nov 27, 2017

View reviewed changes

jd force-pushed the influxdb branch from aa33aee to 8b0b52f Compare November 27, 2017 15:54

chungg previously approved these changes Nov 27, 2017

View reviewed changes

jd dismissed chungg’s stale review via 17ff947 November 27, 2017 17:05

jd force-pushed the influxdb branch from 8b0b52f to 17ff947 Compare November 27, 2017 17:05

jd added 7 commits November 27, 2017 19:15

cli: use --http-socket rather than --http for uwsgi

26844f3

Using --http enables the HTTP router – which is not used here. That router prevents the chunked tranfer encoding to work.

rest: make Prometheus get_or_create_resource_and_metrics() static

d90ab37

rest: make get_or_create_resource_and_metrics() resource type creatio…

740ed02

…n optional

rest: move get_or_create_resource_and_metrics out of the Prometheus c…

8670c26

…lass This is going to be used by other code.

rest: make specific attributes generic in get_or_create_resource_and_…

d07634e

…metrics

api: rename refresh_timeout option to operation_timeout

258d740

This timeout is going to be used as a more general timeout for operations that the API needs to do and that block for a long time.

api: add an InfluxDB API endpoint

b4264ae

This provides a new InfluxDB-compatible endpoint to write data to Gnocchi at `/v1/influxdb'.

jd force-pushed the influxdb branch from 17ff947 to b4264ae Compare November 27, 2017 18:15

chungg approved these changes Nov 27, 2017

View reviewed changes

sileht approved these changes Dec 1, 2017

View reviewed changes

Merge branch 'master' into influxdb

f17980b

ghost merged commit 3a51b86 into gnocchixyz:master Dec 1, 2017

This pull request was closed.

Pull request for influxdb #482

Pull request for influxdb #482

Conversation

jd commented Nov 14, 2017

ghost commented Nov 14, 2017

jd commented Nov 14, 2017

ghost commented Nov 14, 2017

jd commented Nov 15, 2017

ghost commented Nov 15, 2017

jd commented Nov 16, 2017

ghost commented Nov 16, 2017

jd commented Nov 16, 2017

ghost commented Nov 16, 2017

jd commented Nov 16, 2017

ghost commented Nov 16, 2017

jd commented Nov 16, 2017

ghost commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jd commented Nov 23, 2017

ghost commented Nov 23, 2017

jd commented Nov 23, 2017

jd commented Nov 23, 2017

ghost commented Nov 23, 2017

jd commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 27, 2017

jd commented Nov 27, 2017

ghost commented Nov 27, 2017

jd commented Nov 27, 2017

ghost commented Nov 27, 2017

jd commented Nov 27, 2017

ghost commented Nov 27, 2017

sileht left a comment

Choose a reason for hiding this comment

jd commented Dec 1, 2017

sileht commented Dec 1, 2017 • edited Loading

sileht commented Dec 1, 2017 •

edited

Loading