Return new metrics when creating metrics for a resource #55

aalvrz · 2017-05-31T05:59:01Z

Fixes #12

sileht

Also some tests have to be updated.

sileht · 2017-05-31T08:04:59Z

gnocchi/rest/__init__.py

@@ -665,6 +665,8 @@ def post(self):
        except indexer.NoSuchResource as e:
            abort(404, e)

+        return metrics


We should not return the user input, but the values that come from the database. Also we update a resource, so we should return a resource description, not the metrics (the resource description should have the complete metrics info).

Maybe just adding 'return' at the beginning of line 655 is enough.

aalvrz · 2017-05-31T09:00:59Z

Are you referring to the test_post_append_metrics test?

sileht · 2017-05-31T09:11:58Z

On Wed, May 31, 2017 at 02:01:00AM -0700, Andrés Álvarez wrote: Are you referring to the [`test_post_append_metrics`](https://github.com/gnocchixyz/gnocchi/blob/master/gnocchi/tests/test_rest.py#L985) test?

Not only, you can looks at travis-ci job results to see all the failing tests.

…

-- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht

jd

Would be cool to have a different PR for the last commit about doc :)

jd · 2017-05-31T09:56:36Z

gnocchi/rest/__init__.py

@@ -665,6 +665,9 @@ def post(self):
        except indexer.NoSuchResource as e:
            abort(404, e)

+        return pecan.request.indexer.get_resource(


If the user does not need this output it's a whole request for nothing.
It might be better to leave the user request it if it needs it after, no?

Do you mean that the user should somehow specify if the output is needed? Could you show me an example of this?

Not really. What I mean is that if the user wants the new updated resource he just have to do GET /v1/resource/generic/<id>.

And actually returning the entire resource on the /v1/resource/generic/<id>/metric seems wrong to me. This is not the resource endpoint, it's the metric of the resource endpoint. So if anything would be returned it's the new list of metric only – which would be a different SQL request, not sure there's such a call in the indexer API.

Also I did understand at first read you were trying to fix #12. I'd like to have @chungg opionion since he is the one that opened the bug, if this is really a requirement since it will cost an indexer request on each call and I'm not sure it's really worth it.

I agree.

On my first commit before rebasing I was only returning the newly created metrics. But @sileht mentioned something about returning the whole resource's description instead, which would return the new metrics as well.

Waiting on more information to know how to proceed with this.

jd · 2017-05-31T09:56:55Z

gnocchi/tests/test_rest.py

@@ -989,7 +989,7 @@ def test_post_append_metrics(self):
        metrics = {'foo': {'archive_policy_name': "high"}}
        self.app.post_json("/v1/resource/" + self.resource_type
                           + "/" + self.attributes['id'] + "/metric",
-                           params=metrics, status=204)
+                           params=metrics, status=200)


This needs to be in the previous patch IIUC.

sileht · 2017-06-02T08:06:06Z

On my first commit before rebasing I was only returning the newly created metrics. But @sileht mentioned something about returning the whole resource's description, which would return the new metrics as well.

Sorry for that, I have thought this API endpoint was about resource, but it's not.

…

-- Mehdi Abaakouk mail: sileht@sileht.net irc: sileht

aalvrz · 2017-06-05T02:21:02Z

Any consensus on what the response content should be? Or if this change should be abandoned.

jd · 2017-06-05T14:49:26Z

Re-reading this, I think your first patch was correct – @sileht misread the problem as he said after.

You should return the newly metrics that are created, so "return metrics" was the right choice.

But, as @sileht said you can't blindly return "metrics" because it's user-supplied definitions, and not metrics objects from the database.

update_resource() returns the resource with all its metric loaded. So I'd suggest actually returning r.metrics at the end of the function. That'll report all the metrics attached to this resource, which considering the endpoint URL, sounds like a good thing.

aalvrz · 2017-06-06T02:09:39Z

@jd Understood. I have made the changes. Will squash if everything is ok.

sileht · 2017-06-06T04:43:44Z

The solution looks good, maybe we should check the returned json with gabbi in additional to the 204 -> 200 change to make this PR perfect.

aalvrz · 2017-06-06T05:57:10Z

@sileht I have updated the Gabbi tests to check for the JSON response. Please let me know if these are correct and if they are enough.

sileht

Cool thx!

jd · 2017-06-06T06:28:29Z

That's cool, though we need to squash all of that in a single commit. Can you do that @BigChief45? Otherwise I'll try that big UI button in GitHub. :)

aalvrz · 2017-06-06T06:37:59Z

@jd No problem. But I was thinking maybe you could try that big button this time? I've actually never seen it in action myself hehe.

jd · 2017-06-06T06:42:47Z

@BigChief45 Maybe next time, since you'll need to edit that branch to fix the Travis failures. ;)

aalvrz · 2017-06-06T07:25:25Z

@jd No problem. Fixing this soon.

By the way, I am taking a look at the documentation regarding this:

I cannot find the response content/scenario anywhere in the docs. Is this content generated into the docs from an actual request?

jd · 2017-06-06T08:05:36Z

@BigChief45 Yes, the whole API doc is generated from real API calls. Check out https://github.com/gnocchixyz/gnocchi/blob/master/doc/source/rest.yaml and https://github.com/gnocchixyz/gnocchi/blob/master/doc/source/rest.j2

aalvrz · 2017-06-07T01:31:47Z

: Unable to match $[-1].resource_id as 85C44741-CC60-4033-804E-2D3098C7D2E9, got 85c44741-cc60-4033-804e-2d3098c7d2e9

Hm why are resource IDs being passed as uppercase in the requests in the Gabbi tests? Seems a bit strange.

jd · 2017-06-07T14:49:51Z

@BigChief45 Likely because whoever wrote that put them that way. It does not matter and the case should be preserved, so just upper case your changes and you should be all set.

chungg · 2017-06-07T19:55:41Z

gnocchi/rest/__init__.py

@@ -665,6 +667,8 @@ def post(self):
        except indexer.NoSuchResource as e:
            abort(404, e)

+        return r.metrics


i think this will return all metrics in resource not just the new metrics you added. it's failing in gate in gate because the new metric is not necessarily the first item returned.

if you're testing this locally, you can add verbose: true to gabbi tests, so check what order metrics are returned in (i'm guessing alphabetical). something like:

- name: create new metrics verbose: True POST: /v1/resource/generic/f93450f2-d8a5-4d67-9985-02511241e7d1/metric request_headers: x-user-id: 0fbb231484614b1a80131fc22f6afc9c x-project-id: f3d41b770cc14f0bb94a1d5be9c0e3ea content-type: application/json data: foobar: archive_policy_name: low status: 200 response_json_paths: $[0].name: foobar $[0].resource_id: f93450f2-d8a5-4d67-9985-02511241e7d1

This issue is driving me nuts. Locally, all the gabbi tests pass, assuming that the metric being created in the test will be the first one to be returned in the list.

When testing using the Gnocchi API, metrics are not returned in alphabetical order, but in the order of creation (last created, first in the list). But I don't get why it is not the case in the gate.

There's no order set by default. So, by chance it might always been the same order, but it also can depends on whetever it runs on MySQL or PostgreSQL. You can't assume in the test that the order is going to be always the same. Especially because maybe the next release of $RDBMS will break that assumption.

Now, what you can do is sort in the response_json_paths. This is documented here:

http://gabbi.readthedocs.io/en/latest/jsonpath.html

Basically you can do this:

response_json_paths: $[/name][1].name: foobar

If your metric list is "foobar, cpu_util" for example (you'll need to check what the full list is, then "foobar" will be alphabetically after "cpu_util" when sorting by name (the [/name] in the JSON path expression.

aalvrz · 2017-06-08T06:59:32Z

So it seems that the order of the returned metrics differs between Postgresql and MySQL?

aalvrz · 2017-06-09T02:38:31Z

Seems like there is some issue with Docker in the CI.

jd · 2017-06-09T06:19:16Z

That should be fixed now.

Fixes gnocchixyz#12

aalvrz · 2017-06-12T02:51:08Z

I have squashed all the commits and all seems to pass at the gate. I think this is ready to be merged? 😃

Patch updated

jd · 2017-06-12T06:18:10Z

Thanks @BigChief45 !

sileht requested changes May 31, 2017

View reviewed changes

aalvrz force-pushed the fix-#12 branch from e3e0bd9 to 4caa6e4 Compare May 31, 2017 09:45

jd requested changes May 31, 2017

View reviewed changes

jd reviewed May 31, 2017

View reviewed changes

jd requested changes May 31, 2017

View reviewed changes

aalvrz force-pushed the fix-#12 branch from 4caa6e4 to cb41c5c Compare June 1, 2017 02:31

aalvrz force-pushed the fix-#12 branch from 71b6b88 to fb0ae86 Compare June 6, 2017 02:08

aalvrz force-pushed the fix-#12 branch from fb0ae86 to a2569a9 Compare June 6, 2017 02:37

sileht previously approved these changes Jun 6, 2017

View reviewed changes

aalvrz dismissed sileht’s stale review via 9e0ff90 June 6, 2017 05:53

aalvrz force-pushed the fix-#12 branch from 9e0ff90 to 7ae7aba Compare June 6, 2017 05:56

sileht previously approved these changes Jun 6, 2017

View reviewed changes

jd previously approved these changes Jun 6, 2017

View reviewed changes

jd added this to the 4.0 milestone Jun 6, 2017

aalvrz dismissed stale reviews from jd and sileht via 3176295 June 6, 2017 08:37

aalvrz force-pushed the fix-#12 branch from 4e90e9a to 3176295 Compare June 6, 2017 08:37

aalvrz force-pushed the fix-#12 branch from 3176295 to 0ef6ab2 Compare June 6, 2017 08:50

aalvrz force-pushed the fix-#12 branch 2 times, most recently from 7e7edfa to 62f74cc Compare June 7, 2017 03:22

chungg previously requested changes Jun 7, 2017

View reviewed changes

aalvrz force-pushed the fix-#12 branch 2 times, most recently from 5e11ca3 to 1c19ce7 Compare June 9, 2017 01:32

Return new metrics when creating metrics for a resource

8e24800

Fixes gnocchixyz#12

aalvrz force-pushed the fix-#12 branch from fb81423 to 8e24800 Compare June 12, 2017 02:15

jd approved these changes Jun 12, 2017

View reviewed changes

jd merged commit a852437 into gnocchixyz:master Jun 12, 2017

aalvrz deleted the fix-#12 branch June 13, 2017 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return new metrics when creating metrics for a resource #55

Return new metrics when creating metrics for a resource #55

aalvrz commented May 31, 2017

sileht left a comment

sileht May 31, 2017

aalvrz commented May 31, 2017

sileht commented May 31, 2017 via email

jd left a comment •

edited

jd May 31, 2017

aalvrz Jun 1, 2017

jd Jun 1, 2017

aalvrz Jun 2, 2017 •

edited

jd May 31, 2017

sileht commented Jun 2, 2017 via email

aalvrz commented Jun 5, 2017

jd commented Jun 5, 2017

aalvrz commented Jun 6, 2017

sileht commented Jun 6, 2017

aalvrz commented Jun 6, 2017

sileht left a comment

jd commented Jun 6, 2017

aalvrz commented Jun 6, 2017

jd commented Jun 6, 2017

aalvrz commented Jun 6, 2017 •

edited

jd commented Jun 6, 2017

aalvrz commented Jun 7, 2017 •

edited

jd commented Jun 7, 2017

chungg Jun 7, 2017

aalvrz Jun 8, 2017

jd Jun 8, 2017

aalvrz commented Jun 8, 2017

aalvrz commented Jun 9, 2017

jd commented Jun 9, 2017

aalvrz commented Jun 12, 2017

jd commented Jun 12, 2017

Return new metrics when creating metrics for a resource #55

Return new metrics when creating metrics for a resource #55

Conversation

aalvrz commented May 31, 2017

sileht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalvrz commented May 31, 2017

sileht commented May 31, 2017 via email

jd left a comment • edited

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

aalvrz Jun 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sileht commented Jun 2, 2017 via email

aalvrz commented Jun 5, 2017

jd commented Jun 5, 2017

aalvrz commented Jun 6, 2017

sileht commented Jun 6, 2017

aalvrz commented Jun 6, 2017

sileht left a comment

Choose a reason for hiding this comment

jd commented Jun 6, 2017

aalvrz commented Jun 6, 2017

jd commented Jun 6, 2017

aalvrz commented Jun 6, 2017 • edited

jd commented Jun 6, 2017

aalvrz commented Jun 7, 2017 • edited

jd commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalvrz commented Jun 8, 2017

aalvrz commented Jun 9, 2017

jd commented Jun 9, 2017

aalvrz commented Jun 12, 2017

jd commented Jun 12, 2017

jd left a comment •

edited

aalvrz Jun 2, 2017 •

edited

aalvrz commented Jun 6, 2017 •

edited

aalvrz commented Jun 7, 2017 •

edited