It took a `Labels` where the memory could be re-used, but in practice
this hardly ever benefitted. Especially after converting `relabel.Process`
to `relabel.ProcessBuilder`.
Comparing the parameter to `nil` was a bug; `EmptyLabels` is not `nil`
so the slice was reallocated multiple times by `append`.
Lastly `Builder.Labels()` now estimates that the final size will depend
on labels added and deleted.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
We have a LabelBuilder in EvalNodeHelper; use it instead of creating a new one at every step.
Need to take some care that different uses of enh.lb do not overlap.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
We use `labels.Builder` to parse metrics, to avoid depending on the
internal implementation. This is not efficient, but the feature is only
used in tests. It wasn't efficient previously either - calling `Sort()`
after adding each label.
`createLabelsForAbsentFunction` also uses a Builder now, and gets
an extra `map` to replace the previous `Has()` usage.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Fix up promql to compile with changes to Labels
Patterned after `Chunk.Iterator()`: pass the old iterator in so it
can be re-used to avoid allocating a new object.
(This commit does not do any re-use; it is just changing all the method
signatures so re-use is possible in later commits.)
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
If we are populating series for a subquery then set the interval
parameter accordingly so that downstream users could use that
information.
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Use new experimental package `golang.org/x/exp/slices`.
slices.Sort works on values that are directly comparable, like ints,
so avoids the overhad of an interface call to `.Less()`.
Left tests unchanged, because they don't need the speed and it may be
a cross-check that slices.Sort gives the same answer.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* model/relabel: Add benchmark
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* model/relabel: re-use Builder across relabels
Saves memory allocations.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* labels.Builder: allow re-use of result slice
This reduces memory allocations where the caller has a suitable slice available.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* model/relabel: re-use source values slice
To reduce memory allocations.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Unwind one change causing test failures
Restore original behaviour in PopulateLabels, where we must not overwrite the input set.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* relabel: simplify values optimisation
Use a stack-based array for up to 16 source labels, which will be the
vast majority of cases.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* lint
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
We print the stacktrace of a panic when query causes one, but there's no
information about the query itself, which makes it harder to debug and
reproduce the issue.
This adds the 'expr' string to the logged panic.
Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>
* Labels: create signature with/without labels
Instead of creating a new Labels slice then converting to signature,
go directly to the signature and save time.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Labels: refactor Builder tests
Have one test with a range of cases, and have them check the final
output rather than checking the internal structure of the Builder.
Also add a couple of cases where the value is "", which should be
interpreted as 'delete'.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Labels: add 'Keep' function to Builder
This lets us replace `Labels.WithLabels` with the more general `Builder`.
In `engine.resultMetric()` we can call `Keep()` instead of checking
and calling `Del()`.
Avoid calling `Sort()` in `Builder.Labels()` if we didn't add anything,
so that `Keep()` has the same performance as `WithLabels()`.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
For conventional histograms, we need to gather all the individual
bucket timeseries at a data point to do the quantile calculation. The
code so far mirrored this behavior for the new native
histograms. However, since a single data point contains all the
buckets alreade, that's actually not needed. This PR simplifies the
code while still detecting a mix of conventional and native
histograms.
The weird signature calculation for the conventional histograms is
getting even weirder because of that. If this PR turns out to do the
right thing, I will implement a proper fix for the signature
calculation upstream.
Signed-off-by: beorn7 <beorn@grafana.com>
This exactly corresponds to the statistic compared against MaxSamples
during the course of query execution, so users can see how close their
queries are to a limit.
Co-authored-by: Harkishen Singh <harkishensingh@hotmail.com>
Co-authored-by: Andrew Bloomgarden <blmgrdn@amazon.com>
Signed-off-by: Andrew Bloomgarden <blmgrdn@amazon.com>
We always track total samples queried and add those to the standard set
of stats queries can report.
We also allow optionally tracking per-step samples queried. This must be
enabled both at the engine and query level to be tracked and rendered.
The engine flag is exposed via a Prometheus feature flag, while the
query flag is set when stats=all.
Co-authored-by: Alan Protasio <approtas@amazon.com>
Co-authored-by: Andrew Bloomgarden <blmgrdn@amazon.com>
Co-authored-by: Harkishen Singh <harkishensingh@hotmail.com>
Signed-off-by: Andrew Bloomgarden <blmgrdn@amazon.com>
This follows the line of argument that the invariant of not looking
ahead of the query time was merely emerging behavior and not a
documented stable feature. Any query that looks ahead of the query
time was simply invalid before the introduction of the negative offset
and the @ modifier.
Signed-off-by: beorn7 <beorn@grafana.com>
This can happen if the aggregation starts with a float and later
encounters a histogram. In that case, the newly encountered histogram
would have been added to a nil histogram.
This should be tested, of course, but that's best done within the
PromQL testing framework, which we still need to enable for histograms
(for which we have a TODO in the code and now also a card in the GH
project).
Signed-off-by: beorn7 <beorn@grafana.com>
* MergeFloatBucketIterator for []FloatBucketIterator
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
* histogram_quantile for histograms
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
* Fix histogram_quantile
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
* Unit test and enhancements
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
* Iterators to iterate buckets in reverse and all buckets together including zero bucket
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
* Consider all buckets for histogram_quantile and fix the implementation
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
* Remove unneeded code
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
* Fix lint
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
`BufferedSeriesIterator` and `MemoizedSeriesIterator` use a method
called `Values` for exactly the purpose for which all other iterators
of the same kind use a method called `At`. That alone is confusing,
but on top of that, the `Values` method only returns a single sample,
not multiple values. I assume the naming has historical reasons. This
commit makes it more consistent. It is now easier to read, and now
`BufferedSeriesIterator` and `MemoizedSeriesIterator` implement
`chunkenc.Iterator` like many other iterators, too.
Signed-off-by: beorn7 <beorn@grafana.com>
- Pick At... method via return value of Next/Seek.
- Do not clobber returned buckets.
- Add partial FloatHistogram suppert.
Note that the promql package is now _only_ dealing with
FloatHistograms, following the idea that PromQL only knows float
values.
As a byproduct, I have removed the histogramSeries metric. In my
understanding, series can have both float and histogram samples, so
that metric doesn't make sense anymore.
As another byproduct, I have converged the sampleBuf and the
histogramSampleBuf in memSeries into one. The sample type stored in
the sampleBuf has been extended to also contain histograms even before
this commit.
Signed-off-by: beorn7 <beorn@grafana.com>
This is to avoid copying the many fields of a histogram.Histogram all
the time.
This also fixes a bunch of formerly broken tests.
Signed-off-by: beorn7 <beorn@grafana.com>
* Add test case to showcase the problem in #9590
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>
* Don't unwrap ParenExpr in newStepInvariantExpr
Fixes#9590
Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com>
This creates a new `model` directory and moves all data-model related
packages over there:
exemplar labels relabel rulefmt textparse timestamp value
All the others are more or less utilities and have been moved to `util`:
gate logging modetimevfs pool runtime
Signed-off-by: beorn7 <beorn@grafana.com>
* promql: copy data when short-circuiting
Because the range query loop re-uses the output buffer each time round,
we must copy results into the buffer rather than using input as output.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Add benchmark case for many-to-one join
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* query_range: compute join signatures just once
For an expression like `a + on(p,q) b`, extract the `p,q` part from each
series once, instead of re-computing at every step of the range.
Although there was a cache, computing the key by concatenating all
labels was expensive.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Add benchmark for query_range with topk
Modify sample data so values within a metric differ
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Optimise topk where k==1
In this case we don't need a heap to keep track of values; just a single
slot is fine.
Simplify the initialization of the heap: since all cases start off as a
single-item heap we can just assign the value directly.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
* Allow at least one slot in results for topk, quantile
k isn't set for quantile, but we need space to start collecting values
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
In the previous version, 1.18.0, the "megacheck" linter paid attention
to the '//lint:ignore' comment, but that is no longer there.
Newer version pay attention to '//nolint:<linter>,<linter>,...'
comments, optionally followed by a "second" comment introduced by '//'.
Update the directives to use this style.
This is related to prometheus/blackbox_exporter#738 and
prometheus/blackbox_exporter#745.
Signed-off-by: Marcelo E. Magallon <marcelo.magallon@grafana.com>
This commit adds `@ <timestamp>` modifier as per this design doc: https://docs.google.com/document/d/1uSbD3T2beM-iX4-Hp7V074bzBRiRNlqUdcWP6JTDQSs/edit.
An example query:
```
rate(process_cpu_seconds_total[1m])
and
topk(7, rate(process_cpu_seconds_total[1h] @ 1234))
```
which ranks based on last 1h rate and w.r.t. unix timestamp 1234 but actually plots the 1m rate.
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
* promql: Removed global and add ability to have better interval for subqueries if not specified
## Changes
* Refactored tests for better hints testing
* Added various TODO in places to enhance.
* Moved DefaultEvalInterval global to opts with func(rangeMillis int64) int64 function instead
Motivation: At Thanos we would love to have better control over the subqueries step/interval.
This is important to choose proper resolution. I think having proper step also does not harm for
Prometheus and remote read users. Especially on stateless querier we do not know evaluation interval
and in fact putting global can be wrong to assume for Prometheus even.
I think ideally we could try to have at least 3 samples within the range, the same
way Prometheus UI and Grafana assumes.
Anyway this interfaces allows to decide on promQL user basis.
Open question: Is taking parent interval a smart move?
Motivation for removing global: I spent 1h fighting with:
=== RUN TestEvaluations
TestEvaluations: promql_test.go:31: unexpected error: error evaluating query "absent_over_time(rate(nonexistant[5m])[5m:])" (line 687): unexpected error: runtime error: integer divide by zero
--- FAIL: TestEvaluations (0.32s)
FAIL
At the end I found that this fails on most of the versions including this master if you run this test alone. If run together with many
other tests it passes. This is due to SetDefaultEvaluationInterval(1 * time.Minute)
in test that is ran before TestEvaluations. Thanks to globals (:
Let's fix it by dropping this global.
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Added issue links for TODOs.
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Removed irrelevant changes.
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Add errors and Warnings to SeriesSet
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Change Querier interface and refactor accordingly
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Refactor promql/engine to propagate warnings at eval stage
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Address review issues
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Make sure all the series from all Selects are pre-advanced
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Address review issues
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Separate merge series sets
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Clean
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Refactor merge querier failure handling
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Refactored and simplified fanout with improvements from incoming chunk iterator PRs.
* Secondary logic is hidden, instead of weird failed series set logic we had.
* Fanout is well commented
* Fanout closing record all errors
* MergeQuerier improved API (clearer)
* deferredGenericMergeSeriesSet is not needed as we return no samples anyway for failed series sets (next = false).
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Fix formatting
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Fix CI issues
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Added final tests for error handling.
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Addressed Brian's comments.
* Moved hints in populate to be allocated only when needed.
* Used sync.Once in secondary Querier to achieve all-or-nothing partial response logic.
* Select after first Next is done will panic.
NOTE: in lazySeriesSet in theory we could just panic, I think however we can
totally just return error, it will panic in expand anyway.
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Utilize errWithWarnings
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Fix recently introduced expansion issue
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Add tests for secondary querier error handling
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Implement lazy merge
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Add name to test cases
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Reorganize
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Address review comments
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Address review comments
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Remove redundant warnings
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
* Fix rebase mistake
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
* Use go1.14 new hash/maphash to hash both RHS and LHS instead of XOR'ing
which has been resulting in hash collisions.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Refactor engine labelset signature generation, just use labels.Labels
instead of hashes.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Address review comments; function comments + store result of
lhs.String+rhs.String as key.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Replace all signatureFunc usage with signatureFuncString.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Make optimizations to labels String function and generation of rhs+lhs
as string in resultMetric.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Use separate string functions that don't use strconv just for engine
maps.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Use a byte invalid separator instead of quoting and have a buffer
attached to EvalNodeHelper instead of using a global pool in the labels
package.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Address review comments.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Address more review comments, labels has a function that now builds a
byte slice without turning it into a string.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* Use two different non-ascii hex codes as byte separators between labels
and between sets of labels when building bytes of a Labels struct.
Signed-off-by: Callum Styan <callumstyan@gmail.com>
* We only need the 2nd byte invalid sep. at the beginning of a
labels.Bytes
Signed-off-by: Callum Styan <callumstyan@gmail.com>
This addresses fabxc's TODO.
More importantly, it now properly defers the
querier.Close(). Previously, if a panic happened after creation of the
querier within the populateSeries function, querier.Close() was never called.
The latter was responsible for #6977.
Signed-off-by: beorn7 <beorn@grafana.com>
This is technically BREAKING CHANGE, but it was like this from the beginning: I just notice that we rely in
Prometheus on remote read being sorted. This is because we use selected data from remote reads in MergeSeriesSet
which rely on sorting.
I found during work on https://github.com/prometheus/prometheus/pull/5882 that
we do so many repetitions because of this, for not good reason. I think
I found a good balance between convenience and readability with just one method.
Smaller the interface = better.
Also I don't know what TestSelectSorted was testing, but now it's testing sorting.
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
