Support max source resolution for instant queries

[0robustus1]

• [] Added thanos query --query.instant.default.max_source_resolution flag to set a default value for maxSourceResolution on instant queries (as are for example used by the thanos rule component)
• [] Added support for the max_source_resolution query parameter for /api/v1/query

Reasoning

We have configured our compactor with different retentions for different resolution levels:

thanos
  compact
  --log.level=debug
  --data-dir=/var/thanos/store
  --objstore.config-file=/s3/thanos.yaml
  --retention.resolution-raw=7d
  --retention.resolution-5m=30d
  --retention.resolution-1h=180d
  --wait

This causes a situation where the result of a thanos rule recording rule was
only taking the last 7-8 days of data into account, because instant queries were
always using a maxSourceResolution of 0 and therefore only using raw resolution
data.

In our case the query looked like this and was executed on a thanos ruler
because our prometheuses only have a relatively short retention of a few days:

(100 -
  sum_over_time (
    some_metric:errors_count:increase1d[30d]
  ) /
  sum_over_time (
    some_metric:total_count:increase1d[30d]
  ) * 100
)

The thanos rule component runs an instant query in order to retrieve a single
datapoint to set for the recording rule metric. Our query requires that data
from at least 30 days is taken into account. But by default it would only select
raw data and therefore only provide data from the last 7 days.

Changes

This allows for /api/v1/query to accept the ?max_source_resolution query parameter
(the documentation did not clearly state that it was only accepted for
/api/v1/query_range prior to this).
We also added a command line argument to set a default should the query parameter not be set.

We also propose the following (for later PRs):

• Amend the UI to allow setting the max_source_resolution for "Console"
  queries (which are executing instant queries).
• Add a new field to the rule-yaml for thanos ruler to set the
  max_source_resolution on a per-recording-rule basis.

Verification

• We've used a fork to fire individual instant queries with the max_source_resolution
  parameter set and compared it with the last result we got from a range query.
• We also deployed a version with the default value changed and observed that the recording
  rule was now producing the expected value.

[brancz]

This looks right to me, but it'd be good if @bwplotka and or @GiedriusS have a look as well.

[GiedriusS]

The idea to add that parameter there is good but not sure about the flag. Perhaps we could re-use --query.auto-downsampling here and automatically select it depending on the range vectors?

[0robustus1]

Hi @GiedriusS, that might be a significantly more comprehensive and powerful solution.
It sounds however like a larger-scale change (that should then be adopted for all query types).

There is just one concern that comes to mind. Wouldn't the querier need to be aware of the retention configured for the different compaction levels? How else would it have translated the 30d range selector to the correct maxSourceResolution (1h) in a retention setup like this:

thanos
  compact
  --log.level=debug
  --data-dir=/var/thanos/store
  --objstore.config-file=/s3/thanos.yaml
  --retention.resolution-raw=7d
  --retention.resolution-5m=11d
  --retention.resolution-1h=365d
  --wait

Configuring a maxSourceResolution via the flag would enable that.

[povilasv]

Hmm, maybe we should change the default --query.instant.default.max_source_resolution=0s to 1h.

As it has a bit buggy behavior if you configure retention like in the example.

Thoughts?

[0robustus1]

That's what we set it to in our setup now and would probably leave it at. I just wasn't sure whether to include it in the Pull-Request.

[GiedriusS]

Hi @GiedriusS, that might be a significantly more comprehensive and powerful solution.
It sounds however like a larger-scale change (that should then be adopted for all query types).

There is just one concern that comes to mind. Wouldn't the querier need to be aware of the retention configured for the different compaction levels? How else would it have translated the 30d range selector to the correct maxSourceResolution (1h) in a retention setup like this:

thanos
  compact
  --log.level=debug
  --data-dir=/var/thanos/store
  --objstore.config-file=/s3/thanos.yaml
  --retention.resolution-raw=7d
  --retention.resolution-5m=11d
  --retention.resolution-1h=365d
  --wait

Configuring a maxSourceResolution via the flag would enable that.

Indeed, it would be a much bigger change but not sure that introducing a new flag is the best solution. It's always easiest to do that but then after some time you end with a thing which has hundreds of knobs which is bad.

I think my original suggestion would work out because actually we do downsampling at fixed intervals. The options that you've pasted only control the retention part. I have outlined this in my other issue what I have started about how Thanos Store should always prefer higher resolution data.

I still would very much like @bwplotka to comment on this.

[bwplotka]

👋 Thanks for this all!

I am fine with unblocking you @0robustus1 on this, if we make this flag hidden for now. Once that is done LGTM 👍 (: Essentially for long term direction we need bit more design/discussion. IMO there might be some work on PromQL to improve this flow (more below).

Overall, I agree with @GiedriusS and I would vote for query instant asking for range vector using exactly the same logic as the range query. I would really reuse --query.auto-downsampling and same flow.

There is just one concern that comes to mind. Wouldn't the querier need to be aware of the retention configured for the different compaction levels? How else would it have translated the 30d

Technically it would be nice to control all by the step you provide to the query... which you cannot on the instant query. I think there were discussions on enabling step for range vector selector [ .. ] and that would be helpful. I would really love to follow on that with Prometheus team since at some point we want to propose downsampling to Prometheus itself.

Anyway, from Thanos perspective IMO querier should not be aware of anything on storage layer, especially retention - all it knows about is StoreAPI

The options that you've pasted only control the retention part. I have outlined this in my other issue what I have started about how Thanos Store should always prefer higher resolution data.

This is an interesting one. Especially for range vectors and functions, it might be really faster and equally accurate (: in most cases. (: But I don't think for all cases. I think still having granular control for each range vector selector would be the solution here, but let's discuss.

[0robustus1]

I adjusted the code to the remaining review comments. The flag is now hidden.

[bwplotka]

Actually, I was super wrong.. All things I mentioned with resolution per range works right now: https://prometheus.io/blog/2019/01/28/subquery-support/#subqueries

I might think we need to change slightly implementation then for querier and use per selection Step int64 // Query step size in milliseconds. instead of global, query one.

[bwplotka]

Failed quick experiment here so merging this "hidden" workaround for now.

Thanks for doing this! Would love to have your input on further work in this area @0robustus1