tags/findSeries - add lastts-json format

[shanson7]

This is to support efficient querying for missing data.

Fix #1570

[replay]

LGTM

[Dieterbe]

Update http docs please

[shanson7]

I just added a commit that will return partial results if the query is too large. I think this is ok for the findSeries endpoint (the render behavior will still error out). Do you think that I should add an explicit limit to the request (and error if it's too large)? Or return some metadata that indicates the results are truncated?

[Dieterbe]

i'm a bit confused here. doesn't this only include the series name (without tags) ? I thought you wanted it broken down by tags.
also, it looks like this doesn't include meta tags.

[shanson7]

This returns the same string that is currently returned (i.e. the series-json format), which is formatted like name;tag1=val1;tag2=val2

[Dieterbe]

@replay shouldn't we expand the name here to include meta tags?

[replay]

I can't think of a good reason why it would be necessary to include meta tags here, except for consistency so that whoever uses the endpoint also gets to see the meta tags.
Note that in order to include meta tags in this result the index/find_by_tags endpoint and the MemoryIdx.FindByTag() method would all have to be modified to include meta tags in the returned results, which would slow finds down because those results would need to be enriched. So if we want to enable the user to also retrieve the meta tags on this endpoint we should make this an optional feature of the index/find_by_tags endpoint which by default is off.

[shanson7]

Yeah, I would say including meta tags here would be a separate feature.

[replay]

Actually, I just looked at the related code again, and the series returned by clusterFindByTag already have the meta tags if the feature is enabled. They're in the .MetaTags property. So if that's wanted, it would be easy to also add them.

[Dieterbe]

Or return some metadata that indicates the results are truncated?

this.
I think any time any of our http endpoints returns data along with a warning, it should be structured as a dictionary, with a warnings field, like so:

{
    "series": [
        {
            "lastTs": 1576683990,
            "val": "disk.used;datacenter=dc1;rack=a1;server=web01"
        }
    ],
    "warnings": [
        "result set truncated to 1234 items",
    ]
}

shortly we'll start doing the same for render requests too. see grafana/grafana#6448

[Dieterbe]

needs a few tweaks as discussed

[shanson7]

I think any time any of our http endpoints returns data along with a warning, it should be structured as a dictionary, with a warnings field

That's what I was thinking, but I can't really change the existing (and graphite compatible) series-json format to include the warning. I can add the warning element to the lastts format though.

[Dieterbe]

perhaps it should also take a "meta" flag like render responses, which is basically an opt-in to get our "enhanced" response format dictionary which can contain warnings.

[shanson7]

perhaps it should also take a "meta" flag like render responses, which is basically an opt-in to get our "enhanced" response format dictionary which can contain warnings.

It seems like that would be confusing, since for format=lastts-json we could always make meta=true. Perhaps the default should be format=graphite-json or something and that would be the compatibility format, then series-json would be a dictionary format that allows warnings element.

[Dieterbe]

So, we can do:

1. my suggestion:

• lastts-json (default meta=true, users can provide meta=false to remove warnings. or perhaps we don't even allow setting meta=false for this format)
• series-json (default meta=false, users can opt-in)
• "" means series-json

2. your suggestion
   no meta flag

• lastts-json: lastts format with warnings
• graphite-json, "", legacy format without warnings
• series-json: like graphite-json, but includes warnings

I think consistency across our different endpoints is important. It'd be really nice and clean if all endpoints had a "meta" flag which meant the same everywhere (enable metadata and richer response). Though it's annoying to have to opt-in to the better experience (if lastts-json were to default to meta=false), it'd be nicer if enabled by default. But that precludes backwards compatibility (if series-json had meta=true). We can do a comprise, and pick a different meta default based on the format variable. Which seems perhaps a bit strange, but the tradeoff seems worth it.

Proposal 2 is simple, but is different from /render and conflates orthogonal concerns (whether to include metadata/warnings with the formatting of the actual data and its encoding). The data formatting is already conflated with encoding, E.g. What if graphite adds another output format, should we then provide an alternative metadata-enabled format string for that too (and another one when we implement another serialization function)?
Proposal 1 is a bit more complicated (additional meta parameter), but has the benefit it can be applied consistently across all endpoints, I think. (the only inconsistency is whether it's enabled by default, it seems like a more harmless inconsistency than using a completely different metadata-selecting mechanism between different http paths)

So, I have a preference for option 1, but you can probably sway me to 2 :)

[shanson7]

I think consistency across our different endpoints is important. It'd be really nice and clean if all endpoints had a "meta" flag which meant the same everywhere (enable metadata and richer response)

True. I think we really have 2 questions answered in various ways by different parameters:

1. What information should be returned?
2. How should it be encoded?

For the render API, there is a format that is truly just the encoding (json, msgp, pickle, etc). The meta controls what information should be returned. I would argue that the only reason this parameter exists is to maintain graphite compatibility. Otherwise, meta would always be true.

For this endpoint, we envision that there are multiple possible sets of information that would be useful to return (this PR adds 1 of them). The format parameter is answering both questions, and the meta parameter would "enhance" the decision from the first. I'm not sure in what scenario meta=false makes sense except for compatibility.

[Dieterbe]

yes if it weren't for graphite compatibility, we'd always have the rich response and no need for a meta parameter.
Your take makes sense (you say "What information should be returned?" + enhancement through meta, whereas i pulled it out a bit more into separate concerns, saying we need to choose how to format the data, and also how to format the metadata - or rather whether to include metadata - , but it's basically the same thing).

I've stated my conclusion. What's yours?

[shanson7]

I've stated my conclusion. What's yours?

1. lastts-json - ignores meta
2. series-json - compatible return if meta=false else JSON object with series and warnings elements
3. Any new types that are added: Also ignore meta

Sound good?

[Dieterbe]

so basically it's proposal 1 with the tweak that the meta flag only applies to series-json (and "" means series-json); all other formats simply include the metadata.

sounds good

[shanson7]

Excellent, because that's what I just implemented

[Dieterbe]

I think I was able to simplify the wording a bit.
cddafc4
what do you think?

[shanson7]

@Dieterbe - I pulled in your wording changes and cleaned up the little funkiness around limit checks