Downsampling based on MaxResolutionWindow

[eswdd]

Implementation as per comments in #31 - namely only implement where we've been given a non-zero MaxResolutionWindow

[dgl]

I think this also needs to return the chunks in the right bucket, see Raw: at

geras/pkg/store/store.go

Line 545 in 89216b3

Raw: &storepb.Chunk{Type: storepb.Chunk_XOR, Data: c.Bytes()},

and https://github.com/thanos-io/thanos/blob/master/pkg/store/storepb/types.proto#L41

[eswdd]

Oh, half-a-job Simon, will take a look this eve.

[eswdd]

Ugh, so this now gets mucky and there are two solutions. The challenge is working out what bucket we need to put each resulting time series into. IF OpenTSDB supported sending in query ids with sub-queries which we could then reference on response then we could maintain a mapping of id to requested aggregate and then unpack accordingly, BUT it doesn't.

So, there are now two options, both of which rely on using the show_query parameter on the query API, for which I've added support in our OpenTSDB client in a new PR (G-Research/opentsdb-goclient#12)

1. Because there is currently a 1:1 mapping between aggregate type and downsample function, we could parse out the downsample function from the returned results and use this to map back to bucket.
2. We would maintain a mapping between sub-query and aggregate type and then use this to map back later. However if we believe that there won't always be a 1:1 mapping, then we'd potentially want to send multiple query requests as we might not be able to uniquely map everything back later. I've used this approach with Aardvark and it's non-trivial (made worse there because it's all hidden in JavaScript callbacks).

I'm plumping for option 1 for now as we can always switch later.

[eswdd]

@dgl PTAL

[dgl]

Given this is not dependent on external data I'd consider just doing this once in init, then the code doesn't have to pass errors around (just panic, as it will run on startup).

[eswdd]

The rationale for not doing so was to make the code easily reusable in tests, without forcing tests to use the same init.

…

On Mon, 16 Dec 2019 at 10:31, David Leadbeater ***@***.***> wrote: ***@***.**** approved this pull request. ------------------------------ In pkg/store/store.go <#68 (comment)>: > store.updateMetrics(context.Background(), logger) - return store + return store, nil +} + +func (store *OpenTSDBStore) populateMaps() error { Given this is not dependent on external data I'd consider just doing this once in init, then the code doesn't have to pass errors around (just panic, as it will run on startup). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#68?email_source=notifications&email_token=AAMTFG6SEISBUMIW5KOTEPLQY5KI7A5CNFSM4JYJKQG2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPIRQYQ#pullrequestreview-332470370>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMTFG5B5BJ52UTJFJAFUDDQY5KI7ANCNFSM4JYJKQGQ> .

[dgl]

You don’t have to force anything, simply func init in a file will get run for you.

[eswdd]

What is this magic?

…

On Tue, 17 Dec 2019, 09:35 David Leadbeater, ***@***.***> wrote: You don’t have to force anything, simply func init in a file will get run for you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#68?email_source=notifications&email_token=AAMTFGYAKZISVF2LBKOGIALQZCMNFA5CNFSM4JYJKQG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHBYEAI#issuecomment-566460929>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMTFG6UHFUYNPUHN264FEDQZCMNFANCNFSM4JYJKQGQ> .

[eswdd]

I'm not convinced an init() function would be helpful here as that can only init a package level variable, not state within the struct, which then becomes public for all to see, rather than hidden from view..