gnocchixyz / gnocchi

Timeseries database
Apache License 2.0
299 stars 85 forks source link

Allow to specify archive policy when creating measures on batch request #222

Open jd opened 7 years ago

jd commented 7 years ago

When sending measures in a batch with create_metrics=true, it's not possible to specify which archive policy to use:

POST /v1/batch/resources/metrics/measures?create_metrics=true HTTP/1.1
Content-Type: application/json
Content-Length: 200

{
  "ab68da77-fa82-4e67-aba9-270c5a98cbcb": {
    "disk.io.test": [
      {
        "timestamp": "2014-10-06T14:34:12",
        "value": 71
      },
      {
        "timestamp": "2014-10-06T14:34:20",
        "value": 81
      }
    ]
  }
}

Only archive policy rules are used, which is a bummer.

Asu4ni commented 7 years ago

Which kind of implementation is better?

  1. make another parameter in url such as archive_policy=<policy> The disadvantage is that all the new metrics couldn't have different policies.
  2. make it into json content. Like this:
    {
    "ab68da77-fa82-4e67-aba9-270c5a98cbcb": {
    "disk.io.test": {
      archive_policy: <policy>
      measures: [
        {
          "timestamp": "2014-10-06T14:34:12",
          "value": 71
        },
        {
          "timestamp": "2014-10-06T14:34:20",
          "value": 81
        }
      ]
    }
    }
    }
jd commented 7 years ago

@Asu4ni option 2, really :)

Asu4ni commented 7 years ago

I find a problem when I look through the code related to the issue: code     Should we keep the original format as well? If so, when create_metrics = true, the new format is applied. Otherwise, the old one is applied. This sounds horrible and the implementation might be more difficult. Would it be ok to ditch the old format?

jd commented 7 years ago

You cannot break the API. That's impossible, forbidden, nein, nicht, verbotten, interdit, no!

What you can do is add fields or even add a new API endpoint with a new format.

Asu4ni commented 7 years ago

Then I think adding another parameter such as 'specify_archive_policy' should be fine. If it is true, the new format is applied. Would this be what we want?

jd commented 7 years ago

So that's another solution, but in this case we should probably go toward the road of API microversion. I don't have much resources on that but if we invoke @cdent I'm sure he'll have some resources to point us to if we wanted to implement micro-versioning the good way.

cdent commented 7 years ago

Unless you really are eager to go the microversion route (because you expect these sorts of changes to become common) then I would suggest sticking with this:

What you can do is add fields or even add a new API endpoint with a new format.

I'm right in the middle of adding some additional features to the microversion-parse that may be of some use if you do choose to go with them. The implementation in placement is the source for the additions to microversion-parse and is rather more contained and decoupled than some of the other implementations.

The microversion guideline could be helpful, filter out the openstack-ness.

This posting from stripe may also be of use. It talks about a thing that is quite like microversion but without the OpenStack slant.

The important thing to remember, I think, with microversions is that because they help manage change they are effectively a license to make change often. If you need to do that (because you want or need to evolve the api a lot) then great, but for the most part it is better to not change.

leifmadsen commented 2 years ago

We actually ran into an issue with something that was being requested here. If I'm understanding the code correctly, this issue is actually resolved in the latest Gnocchi code, implemented as an optional parameter in this schema definition: https://github.com/gnocchixyz/gnocchi/blob/master/gnocchi/rest/api.py#L1689-L1714

See line https://github.com/gnocchixyz/gnocchi/blob/master/gnocchi/rest/api.py#L1695 in particular.

If I understand this correctly, this issue can be closed as Resolved.

leifmadsen commented 2 years ago

Implemented by this commit: https://github.com/gnocchixyz/gnocchi/commit/081e0513e68fb39630f2abf22fac0e617bce6af6