Fix for issue #1048: Invalid API response returned by mock collector #1049

obourdon · 2016-07-05T10:21:59Z

Fixes #1048

Summary of changes:

using unmodified REST API response data appropriately

Testing done:

small & legacy tests passed using go 1.5.4 on Linux Ubuntu 14.04 (trusty) LTS amd64

@intelsdi-x/snap-maintainers

snapbot · 2016-07-05T10:22:00Z

Can one of the admins verify this patch?

mbbroberg · 2016-07-06T17:30:05Z

@intelsdi-x/snap-maintainers

IRCody · 2016-07-06T20:01:29Z

I think this invalidates some of the help text for snapctl:

% snapctl metric get -h                                                                                                                                                                                                                                                                                              
NAME:
   snapctl metric get - get details on a single metric

USAGE:
   snapctl metric get [command options] [arguments...]

OPTIONS:
   --metric-version value, -v value     The metric version. Default (0) is latest (default: 0)
   --metric-namespace value, -m value   A metric namespace

If you load all the mock plugins and do `snapctl metric get -m "/intel/mock/*" I get:

% snapctl metric get -m "/intel/mock/*"
NAMESPACE                VERSION         UNIT    LAST ADVERTISED TIME            DESCRIPTION
/intel/mock/[host]/baz   1                       Sun, 31 Dec 0000 16:00:00 PST

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT         REQUIRED        MINIMUM         MAXIMUM

NAMESPACE                VERSION         UNIT            LAST ADVERTISED TIME            DESCRIPTION
/intel/mock/[host]/baz   2               mock unit       Wed, 06 Jul 2016 12:45:05 PDT   mock description

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT         REQUIRED        MINIMUM         MAXIMUM

NAMESPACE                VERSION         UNIT    LAST ADVERTISED TIME            DESCRIPTION
/intel/mock/bar          1                       Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT         REQUIRED        MINIMUM         MAXIMUM

NAMESPACE                VERSION         UNIT            LAST ADVERTISED TIME            DESCRIPTION
/intel/mock/bar          2               mock unit       Wed, 06 Jul 2016 12:45:05 PDT   mock description

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT         REQUIRED        MINIMUM         MAXIMUM

NAMESPACE                VERSION         UNIT    LAST ADVERTISED TIME            DESCRIPTION
/intel/mock/foo          1                       Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/foo:

       NAME              TYPE            DEFAULT         REQUIRED        MINIMUM         MAXIMUM
       password          string                          true
       name              string          bob             false

NAMESPACE                VERSION         UNIT            LAST ADVERTISED TIME            DESCRIPTION
/intel/mock/foo          2               mock unit       Wed, 06 Jul 2016 12:45:05 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME              TYPE            DEFAULT         REQUIRED        MINIMUM         MAXIMUM
       name              string          bob             false
       password          string                          true

It is returning multiple versions even though the help text says it will default to the latest. Need to change one or the other so they match.

I'm also wondering if we want to have separate return messages for a single metric vs multiple metrics. Would it make sense to just always return an array of metrics, which may have one or many metrics? Seems like that would simplify things on both server and client side.

nanliu · 2016-07-06T20:03:37Z

I think it makes more sense to return an array of metrics since it simplifies both the server and the client. @tjmcs any thoughts?

tjmcs · 2016-07-13T15:13:35Z

@nanliu, I think it's critical to ensure that even in those cases where we are returning multiple metrics by using a wildcard in the --metric-namespace value we are only returning a single version for each of those metrics (either the latest version or the version specified using a --metric-version value).

From the discussion, above, I think that means the code behind this PR needs to be refactored to ensure that we only get a single value for each metric containing either the latest version or the specified version, not a response containing all versions of all metrics in that namespace. That will ensure that even in those cases where a wildcard is used to retrieve multiple metrics in a given namespace, each of those metrics will match the previous behavior shown by snapctl (and the RESTful API) in those cases where a single metric was being retrieved.

obourdon · 2016-07-13T15:45:36Z

@tjmcs please note that as stated in the issue #1048 the current PR only displays what is currently returned by the REST API. Therefore the modification your are requesting to match current behaviour does not mean refactoring current PR code but has to be fixed elsewhere in the code, unless I missed some point of course

tjmcs · 2016-07-13T16:20:04Z

I disagree to a certain extent, @obourdon. The current behavior is the current behavior. If snapctl is filtering out the response back from the RESTful API in those cases where there is more than one version of a metric (so that only the specified version is displayed or, if no version was specified, the latest version is displayed), then it should do the same thing after this PR is applied (even if that means adding logic to filter the response back from the RESTful API somehow).

That being said, I am not familiar with this part of the codebase, so I'll have to defer to others as to how snapctl currently accomplishes this (so that only one version is displayed for the metric being returned). My only comment was that if that's what snapctl does today, then that's what snapctl should continue to do even after this PR is applied. It shouldn't matter if the namespace passed into snapctl is for a single metric or a set of metrics (by using a wildcard in the namespace), you should only get one version of the metric returned (or of each metric returned). Adding support for wildcards to the metric namespace flag shouldn't change this behavior around which version of the metric (or metrics) is returned.

tjmcs · 2016-07-13T16:25:01Z

As an aside, adding support for a query string to the RESTful API (so that the version can be filtered in the RESTful API when a specific version is given) would definitely be helpful (assuming that such support is not already in the RESTful API somewhere, again I'll have to defer to others on this). My assumption here is that the problem is arising when no version is specified, and that in that situation the RESTful API is returning all available versions of a given metric. I'm also assuming (based on the help output for the snapctl command) that the snapctl command itself is filtering the version in those cases so that only the latest version of a metric is displayed if no version is specified.

candysmurf · 2016-07-15T23:51:25Z

@obourdon, Thanks for your work.

I tested this against the latest master, it seems working as expected. The key here is that you get what you requested for. For example:

Query: snapctl metric get -m "/intel/mock/*"
Result: all metrics along with their versions having prefix "/intel/mock"

Query: snapctl metric get -m "/intel/mock/foo"
Result: returned the latest version

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Fri, 15 Jul 2016 16:29:38 PDT   mock description

Query: snapctl metric get -m "/intel/mock/*/baz"
Result: it returned the latest version

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/[host]/baz   2       mock unit   Fri, 15 Jul 2016 16:29:38 PDT   mock description
 Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

Query: snapctl metric get -m "/intel/mock/foo"
Result: retuned latest

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Fri, 15 Jul 2016 16:29:38 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       password      string              true
       name          string      bob         false

Query: snapctl metric get -m "/intel/mock/bar"
Result: returned latest

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/bar      2       mock unit   Fri, 15 Jul 2016 16:29:38 PDT   mock description

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

This PR is good to go if you can address my other comments.

candysmurf · 2016-07-16T00:01:14Z

cmd/snapctl/metric.go

+		var merr error
+		for i, m := range metric.([]*client.GetMetricResult) {
+			err := printMetric(m, i)
+			if err != nil {


should it be like this - returns an error if one error occurred instead of waiting to the end?

I understand your point however why can't we just go through all potential errors and then return instead ?
The only thing this will change is the fact that all errors will be printed at once instead of only one.

tjmcs · 2016-07-16T00:16:31Z

With the merge of PR #1076 I think this PR can now be merged and provide the expected behavior (only showing the latest version when a version number is not specified). 👍 for a merge from me now

candysmurf · 2016-07-17T19:39:40Z

@obourdon, do you mind to make this PR up to date so that we can get this PR merged? thank you.

obourdon · 2016-07-18T07:02:34Z

@candysmurf will do. However the problem is now that there has been so many changes since I committed this PR, that I have to rework the entire code which I had tested quite extensively. It now does not even compile anymore after merging :-(

candysmurf · 2016-07-18T16:30:48Z

@obourdon, sorry for the inconvenience. I'm looking forward to your update. Thank you.

obourdon · 2016-07-18T17:21:24Z

@candysmurf in fact it was way less painful than I originally estimated ;-). New version uploaded few hours ago

jcooklin · 2016-07-19T06:46:57Z

@tjmcs please review the last commit.

Returns all version if no version is specified, updates snapctl help and updates the href to use a parameter for the namespace query.

tjmcs · 2016-07-19T20:45:37Z

LGTM, @jcooklin and @obourdon; here's the result of my testing of this PR...first a set of tests run using snapctl where the --metric-namespace value both includes and does not include a wildcard and where the --metric-version is not specified (which should default to returning all values of each matching metric), is set to -1 (which should return the latest version of each matching metric), is set to 0 (which should be the same as when the version is not specified), is set to 1 (a number greater than zero that actually exists for each metric we're looking for), and is set to 3 (a metric version that does not exist for any of the metrics being used for testing). First, the results using the snapctl command:

$ snapctl -u "https://127.0.0.1:12345" --insecure metric list
NAMESPACE        VERSIONS
/intel/mock/*/baz    1,2
/intel/mock/bar      1,2
/intel/mock/foo      1,2

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/*'
NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/[host]/baz   1           Sun, 31 Dec 0000 16:00:00 PST

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/[host]/baz   2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/bar      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/bar      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       password      string              true
       name          string      bob         false

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/foo'
NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/*' --metric-version '-1'
NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/[host]/baz   2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/bar      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/*' --metric-version '0'
NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/[host]/baz   1           Sun, 31 Dec 0000 16:00:00 PST

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/[host]/baz   2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/bar      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/bar      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/*' --metric-version '1'
NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/[host]/baz   1           Sun, 31 Dec 0000 16:00:00 PST

  Dynamic elements of namespace: /intel/mock/[host]/baz

       NAME      DESCRIPTION
       host      name of the host

  Rules for collecting /intel/mock/[host]/baz:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/bar      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/bar:

       NAME      TYPE    DEFAULT     REQUIRED    MINIMUM     MAXIMUM

NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/*' --metric-version '3'

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/foo' --metric-version '-1'
NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/foo' --metric-version '0'
NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

NAMESPACE        VERSION     UNIT        LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      2       mock unit   Tue, 19 Jul 2016 13:16:41 PDT   mock description

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       password      string              true
       name          string      bob         false

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/foo' --metric-version '1'
NAMESPACE        VERSION     UNIT    LAST ADVERTISED TIME        DESCRIPTION
/intel/mock/foo      1           Sun, 31 Dec 0000 16:00:00 PST

  Rules for collecting /intel/mock/foo:

       NAME          TYPE        DEFAULT     REQUIRED    MINIMUM     MAXIMUM
       name          string      bob         false
       password      string              true

$ snapctl -u "https://127.0.0.1:12345" --insecure metric get --metric-namespace '/intel/mock/foo' --metric-version '3'

$

and then this equivalent set of curl commands (each correspondiing to one of to the snapctl tests shown above); note that most of these tests use the new style URI's (where the namespace is passed into the GET as a query string parameter), but towards the end I've shown a couple of requests using the old style (where the namespace is included as part of the URI itself) to prove that we still have backwards compatibility (and you'll just have to trust me that all of the cases tested using the new style also work using the old style):

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F*"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/*/baz",
      "version": 1,
      "dynamic": true,
      "dynamic_elements": [
        {
          "index": 2,
          "name": "host",
          "description": "name of the host"
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F%2A%2Fbaz&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/*/baz",
      "version": 2,
      "dynamic": true,
      "dynamic_elements": [
        {
          "index": 2,
          "name": "host",
          "description": "name of the host"
        }
      ],
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F%2A%2Fbaz&ver=2"
    },
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/bar",
      "version": 1,
      "dynamic": false,
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Fbar&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/bar",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Fbar&ver=2"
    },
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/foo",
      "version": 1,
      "dynamic": false,
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "default": "",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/foo",
      "version": 1,
      "dynamic": false,
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "default": "",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F*&ver=-1"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/*/baz",
      "version": 2,
      "dynamic": true,
      "dynamic_elements": [
        {
          "index": 2,
          "name": "host",
          "description": "name of the host"
        }
      ],
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F%2A%2Fbaz&ver=2"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/bar",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Fbar&ver=2"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F*&ver=0"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/*/baz",
      "version": 1,
      "dynamic": true,
      "dynamic_elements": [
        {
          "index": 2,
          "name": "host",
          "description": "name of the host"
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F%2A%2Fbaz&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/*/baz",
      "version": 2,
      "dynamic": true,
      "dynamic_elements": [
        {
          "index": 2,
          "name": "host",
          "description": "name of the host"
        }
      ],
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F%2A%2Fbaz&ver=2"
    },
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/bar",
      "version": 1,
      "dynamic": false,
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Fbar&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/bar",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Fbar&ver=2"
    },
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/foo",
      "version": 1,
      "dynamic": false,
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "default": "",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F*&ver=1"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/*/baz",
      "version": 1,
      "dynamic": true,
      "dynamic_elements": [
        {
          "index": 2,
          "name": "host",
          "description": "name of the host"
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F%2A%2Fbaz&ver=1"
    },
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/bar",
      "version": 1,
      "dynamic": false,
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Fbar&ver=1"
    },
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/foo",
      "version": 1,
      "dynamic": false,
      "policy": [
        {
          "name": "password",
          "type": "string",
          "default": "",
          "required": true
        },
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=1"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F*&ver=3"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": []
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=-1"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=0"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/foo",
      "version": 1,
      "dynamic": false,
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "default": "",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=1"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=1"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": -62135596800,
      "namespace": "/intel/mock/foo",
      "version": 1,
      "dynamic": false,
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "default": "",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=1"
    }
  ]
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=3"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": []
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics/intel/mock/foo?ver=-1"
{
  "meta": {
    "code": 200,
    "message": "Metric returned",
    "type": "metric_returned",
    "version": 1
  },
  "body": {
    "Metric": {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  }
}%

$ curl -k "https://127.0.0.1:12345/v1/metrics/intel/mock/*?ver=-1"
{
  "meta": {
    "code": 200,
    "message": "Metrics returned",
    "type": "metrics_returned",
    "version": 1
  },
  "body": [
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/*/baz",
      "version": 2,
      "dynamic": true,
      "dynamic_elements": [
        {
          "index": 2,
          "name": "host",
          "description": "name of the host"
        }
      ],
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2F%2A%2Fbaz&ver=2"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/bar",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Fbar&ver=2"
    },
    {
      "last_advertised_timestamp": 1468959401,
      "namespace": "/intel/mock/foo",
      "version": 2,
      "dynamic": false,
      "description": "mock description",
      "unit": "mock unit",
      "policy": [
        {
          "name": "name",
          "type": "string",
          "default": "bob",
          "required": false
        },
        {
          "name": "password",
          "type": "string",
          "required": true
        }
      ],
      "href": "https://127.0.0.1:12345/v1/metrics?ns=%2Fintel%2Fmock%2Ffoo&ver=2"
    }
  ]
}%

$

As I said, I'm now seeing consistent behavior across all of these various ways of obtaining the list of metrics, both with and without a wildcard in the metric namespace value so I think this PR is finally ready for a merge.

Not only that, but now that we've removed the inconsistencies in what was returned when the version was set to 0 or was set to a number less than 0 (like -1) between the case where the namespace contains a wildcard and the case where it does not, I think we can mark Issue #1082 as "resolved, no fix necessary" (since we don't have to prevent users from entering version number values that are zero or less than zero to avoid exposing the previous inconsistency in how these versions were handled between these two edge cases...namespaces with and without a wildcard). In place of that issue I think we should add an issue around the need to ensure that the behavior introduced in this PR when it comes to version numbers and what is returned in each case (greater than zero for the specified version number, zero for all versions of a metric, and less than zero for the latest version of a metric) is adequately documented somewhere.

candysmurf · 2016-07-19T21:58:45Z

👍 , LGTM. Special thanks to @tjmcs and @jcooklin for putting in extra effort to enhance the consistent behavior of Snap API. Well done. Thank you!!!

snapbot added tracked and removed tracked labels Jul 6, 2016

jcooklin mentioned this pull request Jul 15, 2016

Fixes #1075 - FetchMetrics doesn't return latest metric(s) when version < 0 #1076

Merged

candysmurf reviewed Jul 16, 2016
View reviewed changes

tjmcs mentioned this pull request Jul 16, 2016

Metric version value passed into a snapctl metric get command is not checked #1082

Closed

Fix for issue #1048: Invalid API response returned by mock collector

3ae67ba

IzabellaRaulin added the reviewed/in-progress label Jul 19, 2016

Adds support for passing the query as a parameter

30478b7

Returns all version if no version is specified, updates snapctl help and updates the href to use a parameter for the namespace query.

tjmcs merged commit 5137f6c into intelsdi-x:master Jul 19, 2016

obourdon deleted the multi-metrics branch September 20, 2016 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #1048: Invalid API response returned by mock collector #1049

Fix for issue #1048: Invalid API response returned by mock collector #1049

obourdon commented Jul 5, 2016

snapbot commented Jul 5, 2016

mbbroberg commented Jul 6, 2016

IRCody commented Jul 6, 2016 •

edited

Loading

nanliu commented Jul 6, 2016

tjmcs commented Jul 13, 2016

obourdon commented Jul 13, 2016 •

edited

Loading

tjmcs commented Jul 13, 2016 •

edited

Loading

tjmcs commented Jul 13, 2016

candysmurf commented Jul 15, 2016 •

edited

Loading

candysmurf Jul 16, 2016

obourdon Jul 18, 2016

candysmurf Jul 18, 2016

tjmcs commented Jul 16, 2016 •

edited

Loading

candysmurf commented Jul 17, 2016

obourdon commented Jul 18, 2016

candysmurf commented Jul 18, 2016

obourdon commented Jul 18, 2016

jcooklin commented Jul 19, 2016

tjmcs commented Jul 19, 2016

candysmurf commented Jul 19, 2016

Fix for issue #1048: Invalid API response returned by mock collector #1049

Fix for issue #1048: Invalid API response returned by mock collector #1049

Conversation

obourdon commented Jul 5, 2016

snapbot commented Jul 5, 2016

mbbroberg commented Jul 6, 2016

IRCody commented Jul 6, 2016 • edited Loading

nanliu commented Jul 6, 2016

tjmcs commented Jul 13, 2016

obourdon commented Jul 13, 2016 • edited Loading

tjmcs commented Jul 13, 2016 • edited Loading

tjmcs commented Jul 13, 2016

candysmurf commented Jul 15, 2016 • edited Loading

candysmurf Jul 16, 2016

Choose a reason for hiding this comment

obourdon Jul 18, 2016

Choose a reason for hiding this comment

candysmurf Jul 18, 2016

Choose a reason for hiding this comment

tjmcs commented Jul 16, 2016 • edited Loading

candysmurf commented Jul 17, 2016

obourdon commented Jul 18, 2016

candysmurf commented Jul 18, 2016

obourdon commented Jul 18, 2016

jcooklin commented Jul 19, 2016

tjmcs commented Jul 19, 2016

candysmurf commented Jul 19, 2016

IRCody commented Jul 6, 2016 •

edited

Loading

obourdon commented Jul 13, 2016 •

edited

Loading

tjmcs commented Jul 13, 2016 •

edited

Loading

candysmurf commented Jul 15, 2016 •

edited

Loading

tjmcs commented Jul 16, 2016 •

edited

Loading