query: fix querying with interleaved data #5035
Conversation
|
cc @fpetkovski |
Fix querying when some data has been pushed down and some - hasn't.
Since `max_over_time` and `min_over_time` remove `__name__` from the
results either way, let's do that inside Select() to have a unified form
of data.
Add test to cover this case.
Without this code, the test fails:
```
level=error ts=2022-01-06T16:28:37.959956685Z msg="function failed. Retrying in next tick" err="read query instant response: expected 2xx response, got 422. Body: {\"status\":\"error\",\"errorType\":\"execution\",\"error\":\"vector cannot contain metrics with the same labelset\"}\n"
```
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
GiedriusS
force-pushed
the
attach_metrics_name
branch
from
62bef32 to
fac4e88
Compare
| testutil.Ok(t, err) | ||
| t.Cleanup(e2ethanos.CleanScenario(t, e)) | ||
|
|
||
| prom1, sidecar1, err := e2ethanos.NewPrometheusWithSidecar(e, "p1", defaultPromConfig("p1", 0, "", ""), "", e2ethanos.DefaultPrometheusImage(), "remote-write-receiver") |
There was a problem hiding this comment.
Is it intended to enable remote write receiver?
There was a problem hiding this comment.
The test synthesizes samples by remote writing them, which is why the receiver feature is needed.
There was a problem hiding this comment.
Thanks @fpetkovski. I thought it was using data from the block only. Didn't notice the remote write part.
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
| // Delete the metric's name from the result because that's what the | ||
| // PromQL does either way and we want our iterator to work with data | ||
| // that was either pushed down or not. | ||
| if q.enableQueryPushdown && (hints.Func == "max_over_time" || hints.Func == "min_over_time") { |
There was a problem hiding this comment.
Just curious why promql does this. Isn't it a bug?
There was a problem hiding this comment.
Nope, it is a feature. Think of these functions like they are calculating the maximum over all input, not just for each unique labelset. I think this is where it happens: https://github.com/prometheus/prometheus/blob/d677aa4b29a8a0cf9f61af04bbf5bfdce893cf23/promql/engine.go#L1304-L1310.
There was a problem hiding this comment.
Once more functions are being pushed down, we will need to improve the logic here. Probably we will need to refactor this into a separate struct? I don't know yet
Fix querying when some data has been pushed down and some - hasn't.
Since
max_over_timeandmin_over_timeremove__name__from theresults either way, let's do that inside Select() to have a unified form
of data.
Add test to cover this case.
Without this code, the test fails:
Closes #5033.