Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add query hints to remote read #2580

Closed
brian-brazil opened this Issue Apr 5, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@brian-brazil
Copy link
Member

brian-brazil commented Apr 5, 2017

As discussed back at promcon, we should add a map[string]string with hints like the function wrapping the selector, the range and the step. This is unstructured and experimental as we don't know yet what if any of this will prove useful for things like downsampling optimisations on the other end.

At some point (likely after the rest of remote read is considered non-experimental), this should be changed to proper structured data when we have a good idea from multiple implementations what information is needed.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jan 9, 2018

I don't see an immediate benefit to making it unstructured initially. If you change something, the other end won't work anymore either way. Just that you won't notice it at compile time with map[string]string.

None is more or less work than the other really. You can iterate just as well on structured info.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jan 9, 2018

This would be another argument to Select I assume, like Select(*Params, ...labels.Matcher)?

What I can see immediately would be:

type Aggregation int

const (
    AggrNone Aggregation = iota
    AggrAvg
    AggrMin
    AggrMax
    AggrCounter
    AggrCount
)

type Param struct {
    Step int64
    Aggr Aggregation
}

The aggregation would make some assumptions already about what a given PromQL function needs. We could just make it a function name of course. But I'd believe that every implementation making use of these would just end up mapping our dozens of functions theirselves, which introduces lots of room for error.

@brian-brazil

This comment has been minimized.

Copy link
Member Author

brian-brazil commented Jan 9, 2018

I don't think we should make any assumptions about how downsampling works and what information it finds useful for a first version. Thus why I propose passing it down and only finalising this API after there's more experience.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jan 9, 2018

Then just the raw info

type Params struct {
    Step int64
    Func string
}

Strictly speaking sum/avg/... are not functions. Should this go into an extra Aggr string field?

@brian-brazil

This comment has been minimized.

Copy link
Member Author

brian-brazil commented Jan 9, 2018

There's no overlap between function names and operator names, so for a first pass I'd suggest putting them all in one field.

@henridf

This comment has been minimized.

Copy link
Contributor

henridf commented Apr 25, 2018

I'd like to work on this - I've started taking a look and I'll report back here if I run into anything, otherwise will hopefully have a PR soon.

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 22, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 22, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.