add flag max query samples

[yeya24]

Signed-off-by: yeya24 yb532204897@gmail.com

• [] CHANGELOG entry if change is relevant to the end user.

Changes

fix #703

Verification

[GiedriusS]

Thanks for working on this! Very appreciated. In the original issue @bwplotka says:

At first glance, yes as we do max sample verify while we are iterating over messages from stream, so we will not fetch more much more samples than query.max-samples define.

Is this really true? Could you or someone point me to code where this happens? I'm afraid that the PromQL engine only sees the "end results", after the Select() gets everything from StoreAPI nodes and, optionally, deduplicates them.

[GiedriusS]

@yeya24 do you have any comments on my previous question? @bwplotka WDYT?

[yeya24]

@yeya24 do you have any comments on my previous question? @bwplotka WDYT?

I am sorry that I forget this pr... I will look through it further and respond it

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[yeya24]

/reopen

[bwplotka]

Just ask us (: Or help us maintaining Thanos and become Thanos maintainer or Triage (: Would you be interested @yeya24 ? (:

[bwplotka]

LGTM 👍

Thanks!

[GiedriusS]

What about my comment? AIUI this limit is only being applied after the results from Select() have been retrieved. The Select() called is this one: https://github.com/thanos-io/thanos/blob/master/pkg/query/querier.go#L168. Inside of it, we already retrieve all of the results and only then pass them on to the PromQL engine which actually applies those limits. I tested out with --query.max-samples=1 and a query sum(up) by (job). Added a small fmt.Println there: fmt.Println(len(resp.seriesSet)) and it prints out numbers like 7596 which means AIUI that those series have been loaded but they haven't been passed to the engine itself. After that, I get that error but that data is still in my querier. Shouldn't we document this better at the very least so that we wouldn't mislead our users? Or perhaps I am wrong.

[yeya24]

@GiedriusS Thanks for the comment. I think you are right, these samples from Select() are already in the Querier's memory and the help message in this flag is quite misleading. What about Maximum number of samples in a single query evaluation? Do you think should we change the name as well?

[bwplotka]

Sorry, I totally missed this. You are right @GiedriusS I believe in this case we even have to calculate by estimating chunks again when they appear on stream... 🤔

[GiedriusS]

Yep, that's why I was hesitant to merge this. Maybe let's revert this current version and remake it into something that uses a counter on the Recv() part? I feel like no matter how we would name this, it wouldn't bring much benefit because AIUI the main objective is to prevent OOM situations but currently it almost doesn't help with that :/ and I just want to avoid getting lots of tickets on github about how this option doesn't work

[bwplotka]

Agree - let's revert. Sorry my bad - premature approve.

[GiedriusS]

Agree - let's revert. Sorry my bad - premature approve.

It's ok, no worries! 😄