Idea: add correctness tests

[GiedriusS]

Between the query that the users send to Thanos and what is actually used in the code some translation happens. For example, some translation happens here https://github.com/thanos-io/thanos/blob/55cb8ca38b3539381dc6a781e637df15c694e50a/pkg/store/matchers.go and

thanos/pkg/query/querier.go

Lines 151 to 171 in 0bb67bc

	// aggrsFromFunc infers aggregates of the underlying data based on the wrapping
	// function of a series selection.
	func aggrsFromFunc(f string) ([]storepb.Aggr, resAggr) {
	if f == "min" || strings.HasPrefix(f, "min_") {
	return []storepb.Aggr{storepb.Aggr_MIN}, resAggrMin
	}
	if f == "max" || strings.HasPrefix(f, "max_") {
	return []storepb.Aggr{storepb.Aggr_MAX}, resAggrMax
	}
	if f == "count" || strings.HasPrefix(f, "count_") {
	return []storepb.Aggr{storepb.Aggr_COUNT}, resAggrCount
	}
	// f == "sum" falls through here since we want the actual samples.
	if strings.HasPrefix(f, "sum_") {
	return []storepb.Aggr{storepb.Aggr_SUM}, resAggrSum
	}
	if f == "increase" || f == "rate" {
	return []storepb.Aggr{storepb.Aggr_COUNTER}, resAggrCounter
	}
	// In the default case, we retrieve count and sum to compute an average.
	return []storepb.Aggr{storepb.Aggr_COUNT, storepb.Aggr_SUM}, resAggrAvg

. There might be bugs in that code. It would be nice to have a good way to test it out with a live Prometheus to see if everything really works as expected.

This came up from #2245. We could protect ourselves against this by running some kind of correctness tests.

Some ideas below.

We could add another e2e scenario where a block would be generated with known data that then would be added to a Prometheus instance that has Sidecar in front of it + uploaded to some remote storage that is picked up by Thanos Store. Then, we could run some (randomly generated) queries and see if we get the same results. In all of the cases, they should be the same if everything is correct.

I haven't looked into the Prometheus side of this too much but I have found that there is some kind of PromQL test suite: https://github.com/prometheus/prometheus/tree/master/promql/testdata. Perhaps we could adapt it and reuse the same?

Cortex has this A/B testing tool cortexproject/cortex#2203. It could also be extended to include correctness tests.

Perhaps there is some kind of PromQL fuzzer that I am not aware of? :P Downsampled data probably shouldn't be a part of this.

Thoughts? :)

/cc @yeya24

[yeya24]

Is it doable in our e2e? We can start one Promethues, one sidecar and a querier. And simply compare the query results of Prometheus and querier.

https://github.com/prometheus/prometheus/tree/master/promql/testdata is great. But is it easy to add the metrics we need into the e2e Prometheus? The testdata is coupled with https://github.com/prometheus/prometheus/blob/84b4d079c8714be8e8ad071a35b0391df270364c/promql/test.go#L65:1 test framework.

Maybe we can extend the thanosbench. Put all the functionalities there.

[brancz]

I think this type of thing is typically referred to as model based testing. I'm 100% for this! Thanos receive definitely could benefit from testing like this a lot as well! (eg. comparing single node vs sharded+replicated setups)

I think this would make a nice GSoC project as well, and could even teach students about distributed systems correctness.

[squat]

I think that in particular correctness of the interaction between compactor and stores would GREATLY benefit from this. There is a lot of subtlety in the interaction between all of the different metas, deletion markers etc

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[GiedriusS]

Still valid.

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[GiedriusS]

https://promlabs.com/blog/2020/08/06/comparing-promql-correctness-across-vendors tests were performed here but I guess it'd be nice to have something in the CI pipeline as well

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[GiedriusS]

No updates from me :/ still valid IMHO

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[GiedriusS]

Actually, #3631 is a start for this :P It's a bit different but still a step towards having automatic tests for PromQL compliance.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[stale]

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

[yeya24]

Is this still valid? There is promql compliance test

[GiedriusS]

Don't think so, closing!