[BUGFIX] Storage: errors from a single secondary querier should be warnings.
This is a backport of #14496 to release-2.54 branch.
#13434 introduced an unwanted change in behaviour: if there was no primary querier and a single secondary querier, the secondary would be treated like a primary. This PR restores the previous behaviour, that all secondary queriers report errors as warnings.
In order to test this behaviour, I changed `TestMergeQuerierWithSecondaries_ErrorHandling` so it now calls `NewMergeQuerier` rather than creating the internal data structure directly.
This in turn required all the data types to change, so I merged `mockGenericQuerier` into `mockQuerier`.
Also replaced `unwrapMockGenericQuerier` with a visitor pattern.
While I was there, I addressed the comment from https://github.com/prometheus/prometheus/pull/13434#pullrequestreview-2191058921 to short-circuit the merge of single querier with any number of no-op or nil queriers.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Believed to trigger segmentation faults due to memory-mapped block
data still being accessed by iterators after the querier is closed.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
The histogram stats decoder keeps track of the last seen histogram sample
in order to properly detect counter resets. We are seeing an issue where
a histogram with UnknownResetHint gets treated as a counter reset when it follows
a stale histogram sample.
I believe that this is incorrect since stale samples should be completely ignored
in PromQL. As a result, they should not be stored in the histogram stats iterator
and the counter reset detection needs to be done against the last non-stale sample.
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
I often see people ask questions that indicate they don't understand
this point, and launching into "instant vector" and "range vector" is
likely to point them in the wrong direction.
Remove the admonishment that the reader mustn't confuse these things.
Remove mention of "inferred sample timestamps" that is never explained.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
`TestMergeQuerierWithSecondaries_ErrorHandling` now tests `NewMergeQuerier`
rather than creating the data structure directly. This means we now test
short-circuiting when only a single querier is required.
Merge `mockGenericQuerier` into `mockQuerier`.
Replace `unwrapMockGenericQuerier` with a visitor pattern.
No change in functionality intended.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Rules: Refactor concurrency controller interface
Even though the main purpose of this refactor is to modify the interface of the concurrency controller to accept a Context. I did two drive-by modifications that I think are sensible:
1. I have moved the check for dependencies on rules to the controller itself - this aligns with how the controller should behave as it is a deciding factor on wether we should run concurrently or not.
2. I cleaned up some unused methods from the days of the old interface before #13527 changed it.
Signed-off-by: gotjosh <josue.abreu@gmail.com>
---------
Signed-off-by: gotjosh <josue.abreu@gmail.com>
Previously, the config doc suggested that scrapes are simply failed if
the bucket limit is exceeded. However, instead Prometheus first
attempts to reduce the resolution. This will succeed in almost all
non-pathological cases. However, in the rare case that it doesn't, the
scrape will be failed after all.
For the resolution limit AKA `native_histogram_min_bucket_factor`, the
logic is a bit different. Once the highest factor, i.e. schema -4, is
reached, the scrape will still succeed.
Signed-off-by: beorn7 <beorn@grafana.com>
