Commit graph

800 commits

Author SHA1 Message Date
Bryan Boreham e4dd3469ac lint
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-08-28 10:39:16 +01:00
Bryan Boreham 5ce990cabc promql: simplify rangeEval a bit more
We can't have both a float and a histogram at the same timestep.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-08-28 10:28:09 +01:00
Bryan Boreham 3879488476 promql: simplify inner loop of rangeEval
Took out the loops with break after one iteration, and extract some
common code to a function.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-08-21 19:52:14 +01:00
Michael Hoffmann 4d8e380269
promql: allow tests to be imported (#12050)
Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
2023-08-18 20:48:59 +02:00
Bryan Boreham 5cea37c069
Merge pull request #12682 from bboreham/contains-same-label-set
promql engine: check unique labels using existing map

ContainsSameLabelset constructs a map with the same hash key as the one used to compile the output of rangeEval, so we can use that one and save work.

Need to hold the timestamp so we can be sure we saw the same series in the same evaluation.
2023-08-14 14:12:47 +01:00
Bryan Boreham 0670e4771a promql engine: check unique labels using existing map
`ContainsSameLabelset` constructs a map with the same hash key as
the one used to compile the output of `rangeEval`, so we can use that
one and save work.

Need to hold the timestamp so we can be sure we saw the same series
in the same evaluation.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-08-13 18:09:10 +01:00
Bryan Boreham 8d47b3d497
Merge pull request #12579 from charleskorn/timestamp
Don't recreate iterator for each series on each timestep when evaluating a query with `timestamp()`
2023-08-05 10:51:38 +01:00
Charles Korn d396282941
Address PR feedback: clarify comment
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-08-02 11:48:34 +10:00
Charles Korn 145d7457fe
Address PR feedback: use loop to create expected test result
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-08-01 13:30:12 +10:00
Charles Korn 6087c555ed
Address PR feedback: clarify comment
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-08-01 13:30:10 +10:00
Charles Korn fb3935e8f9
Address PR feedback: rename method
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-08-01 13:30:07 +10:00
Julius Volz 531567d46e Drop metric name for "atan2" binary operator
The operator changes the meaning of the metric, so the metric name should
be dropped. Technically this would be a breaking change, but it's also very
obviously a bug and not likely that anyone depends on it.

Signed-off-by: Julius Volz <julius.volz@gmail.com>
2023-07-24 14:36:02 +02:00
Charles Korn 6903d6edd8
Add test to confirm timestamp() behaves correctly when evaluating a range query.
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:25:33 +10:00
Charles Korn fde6ebb17d
Create per-series iterators only once per selector, rather than recreating it for each time step.
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:24:21 +10:00
Charles Korn 993618adea
Don't create a new iterator for every time step.
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:24:21 +10:00
Charles Korn b114c0888d
Simplify loop
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:24:20 +10:00
Charles Korn a142998052
Expand series set just once
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:24:19 +10:00
Charles Korn eeface2e17
Inline method
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:24:19 +10:00
Charles Korn a2a2cc757e
Extract timestamp special case to its own method.
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:24:18 +10:00
Charles Korn 15fa680117
Add benchmark for query using timestamp()
Signed-off-by: Charles Korn <charles.korn@grafana.com>
2023-07-20 11:24:16 +10:00
Julien Pivotto 0a48f93111
Merge pull request #10367 from ianwoolf/pr_add_close_for_query_logger
add Close for ActiveQueryTracker to close the file.
2023-07-18 13:53:18 +02:00
cui fliter 096ceca44f
remove repetitive words (#12556)
Signed-off-by: cui fliter <imcusg@gmail.com>
2023-07-13 15:53:40 +02:00
beorn7 162612ea86 histograms: Improve comment
Oversight during review of #12525.

Signed-off-by: beorn7 <beorn@grafana.com>
2023-07-12 14:52:49 +02:00
Ziqi Zhao 42d9169ba1 enhance histogram_quantile to get min/max value
Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
2023-07-12 04:29:54 +08:00
Carrie Edwards 2f9bc98b8a Add tests for min and max functions
Signed-off-by: Carrie Edwards <edwrdscarrie@gmail.com>
2023-07-11 21:51:20 +08:00
Carrie Edwards bc0ee4a469 Implement native histogram min and max query functions
Signed-off-by: Carrie Edwards <edwrdscarrie@gmail.com>
2023-07-11 21:51:20 +08:00
Bryan Boreham ce153e3fff Replace sort.Sort with faster slices.SortFunc
The generic version is more efficient.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-07-10 09:43:45 +00:00
Giedrius Statkevičius 3f230fc9f8 promql: convert QueryOpts to interface
Convert QueryOpts to an interface so that downstream projects like
https://github.com/thanos-community/promql-engine could extend the query
options with engine specific options that are not in the original
engine.

Will be used to enable query analysis per-query.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
2023-07-03 16:20:31 +03:00
Julien Pivotto a605b81b14
Merge pull request #12170 from fpetkovski/parser-inject-functions
parser: Allow parsing arbitrary functions
2023-06-27 13:32:46 +02:00
Bryan Boreham 67d2ef004d Placate lint
I think the version using scoping was better, but I'm out of energy to fight the linter.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-06-01 18:36:34 +00:00
Bryan Boreham bb0d8320dd promql: include parsing in active-query tracking
So that the max-concurrency limit is applied.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-06-01 18:16:05 +00:00
Bryan Boreham 71fc4f1516 promql: refactor: create query object before parsing
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-06-01 17:54:17 +00:00
Bryan Boreham 1f3821379c promql: refactor: extract fn to wait on concurrency limit
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
2023-06-01 17:17:04 +00:00
zenador 191bf9055b
Handle more arithmetic operators for native histograms (#12262)
Handle more arithmetic operators and aggregators for native histograms

This includes operators for multiplication (formerly known as scaling), division, and subtraction. Plus aggregations for average and the avg_over_time function.

Stdvar and stddev will (for now) ignore histograms properly (rather than counting them but adding a 0 for them).

Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
2023-05-16 21:15:20 +02:00
beorn7 9e500345f3 textparse/scrape: Add option to scrape both classic and native histograms
So far, if a target exposes a histogram with both classic and native
buckets, a native-histogram enabled Prometheus would ignore the
classic buckets. With the new scrape config option
`scrape_classic_histograms` set, both buckets will be ingested,
creating all the series of a classic histogram in parallel to the
native histogram series. For example, a histogram `foo` would create a
native histogram series `foo` and classic series called `foo_sum`,
`foo_count`, and `foo_bucket`.

This feature can be used in a migration strategy from classic to
native histograms, where it is desired to have a transition period
during which both native and classic histograms are present.

Note that two bugs in classic histogram parsing were found and fixed
as a byproduct of testing the new feature:

1. Series created from classic _gauge_ histograms didn't get the
   _sum/_count/_bucket prefix set.
2. Values of classic _float_ histograms weren't parsed properly.

Signed-off-by: beorn7 <beorn@grafana.com>
2023-05-13 01:32:25 +02:00
Justin Lei 7bbf24b707 Make MemoizedSeriesIterator not implement chunkenc.Iterator
Signed-off-by: Justin Lei <justin.lei@grafana.com>
2023-05-03 12:45:39 -07:00
Justin Lei 6985dcbe73 Optimize and test MemoizedSeriesIterator
Signed-off-by: Justin Lei <justin.lei@grafana.com>
2023-05-02 08:53:18 -07:00
Matthieu MOREL 7e9acc2e46
golangci-lint: remove skip-cache and restore singleCaseSwitch rule
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
2023-04-20 18:43:51 +02:00
Julien Pivotto f7c6130ff2
Merge pull request #12251 from prymitive/query_samples_total
Add query_samples_total metric
2023-04-20 15:48:24 +02:00
Matthieu MOREL bae9a21200
Merge branch 'main' into linter/nilerr
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
2023-04-19 19:56:39 +02:00
beorn7 5b53aa1108 style: Replace else if cascades with switch
Wiser coders than myself have come to the conclusion that a `switch`
statement is almost always superior to a statement that includes any
`else if`.

The exceptions that I have found in our codebase are just these two:

* The `if else` is followed by an additional statement before the next
  condition (separated by a `;`).
* The whole thing is within a `for` loop and `break` statements are
  used. In this case, using `switch` would require tagging the `for`
  loop, which probably tips the balance.

Why are `switch` statements more readable?

For one, fewer curly braces. But more importantly, the conditions all
have the same alignment, so the whole thing follows the natural flow
of going down a list of conditions. With `else if`, in contrast, all
conditions but the first are "hidden" behind `} else if `, harder to
spot and (for no good reason) presented differently from the first
condition.

I'm sure the aforemention wise coders can list even more reasons.

In any case, I like it so much that I have found myself recommending
it in code reviews. I would like to make it a habit in our code base,
without making it a hard requirement that we would test on the CI. But
for that, there has to be a role model, so this commit eliminates all
`if else` occurrences, unless it is autogenerated code or fits one of
the exceptions above.

Signed-off-by: beorn7 <beorn@grafana.com>
2023-04-19 17:22:31 +02:00
beorn7 c3c7d44d84 lint: Adjust to the lint warnings raised by current versions of golint-ci
We haven't updated golint-ci in our CI yet, but this commit prepares
for that.

There are a lot of new warnings, and it is mostly because the "revive"
linter got updated. I agree with most of the new warnings, mostly
around not naming unused function parameters (although it is justified
in some cases for documentation purposes – while things like mocks are
a good example where not naming the parameter is clearer).

I'm pretty upset about the "empty block" warning to include `for`
loops. It's such a common pattern to do something in the head of the
`for` loop and then have an empty block. There is still an open issue
about this: https://github.com/mgechev/revive/issues/810 I have
disabled "revive" altogether in files where empty blocks are used
excessively, and I have made the effort to add individual
`// nolint:revive` where empty blocks are used just once or twice.
It's borderline noisy, though, but let's go with it for now.

I should mention that none of the "empty block" warnings for `for`
loop bodies were legitimate.

Signed-off-by: beorn7 <beorn@grafana.com>
2023-04-19 17:10:10 +02:00
Ben Ye fd3630b9a3 add ctx to QueryEngine interface
Signed-off-by: Ben Ye <benye@amazon.com>
2023-04-17 21:32:38 -07:00
ianwoolf 79e4bdee8e add Close for ActiveQueryTracker to close the file.
Signed-off-by: ianwoolf <btw515wolf2@gmail.com>
2023-04-14 14:43:23 +08:00
Matthieu MOREL fb3eb21230 enable gocritic, unconvert and unused linters
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
2023-04-13 19:20:22 +00:00
beorn7 551de0346f promql: Do not return nil slices to the pool
Signed-off-by: beorn7 <beorn@grafana.com>
2023-04-13 19:25:24 +02:00
beorn7 817a2396cb Name float values as "floats", not as "values"
In the past, every sample value was a float, so it was fine to call a
variable holding such a float "value" or "sample". With native
histograms, a sample might have a histogram value. And a histogram
value is still a value. Calling a float value just "value" or "sample"
or "V" is therefore misleading. Over the last few commits, I already
renamed many variables, but this cleans up a few more places where the
changes are more invasive.

Note that we do not to attempt naming in the JSON APIs or in the
protobufs. That would be quite a disruption. However, internally, we
can call variables as we want, and we should go with the option of
avoiding misunderstandings.

Signed-off-by: beorn7 <beorn@grafana.com>
2023-04-13 19:25:24 +02:00
beorn7 c0879d64cf promql: Separate Point into FPoint and HPoint
In other words: Instead of having a “polymorphous” `Point` that can
either contain a float value or a histogram value, use an `FPoint` for
floats and an `HPoint` for histograms.

This seemingly small change has a _lot_ of repercussions throughout
the codebase.

The idea here is to avoid the increase in size of `Point` arrays that
happened after native histograms had been added.

The higher-level data structures (`Sample`, `Series`, etc.) are still
“polymorphous”. The same idea could be applied to them, but at each
step the trade-offs needed to be evaluated.

The idea with this change is to do the minimum necessary to get back
to pre-histogram performance for functions that do not touch
histograms. Here are comparisons for the `changes` function. The test
data doesn't include histograms yet. Ideally, there would be no change
in the benchmark result at all.

First runtime v2.39 compared to directly prior to this commit:

```
name                                                  old time/op    new time/op    delta
RangeQuery/expr=changes(a_one[1d]),steps=1-16            391µs ± 2%     542µs ± 1%  +38.58%  (p=0.000 n=9+8)
RangeQuery/expr=changes(a_one[1d]),steps=10-16           452µs ± 2%     617µs ± 2%  +36.48%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_one[1d]),steps=100-16         1.12ms ± 1%    1.36ms ± 2%  +21.58%  (p=0.000 n=8+10)
RangeQuery/expr=changes(a_one[1d]),steps=1000-16        7.83ms ± 1%    8.94ms ± 1%  +14.21%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_ten[1d]),steps=1-16           2.98ms ± 0%    3.30ms ± 1%  +10.67%  (p=0.000 n=9+10)
RangeQuery/expr=changes(a_ten[1d]),steps=10-16          3.66ms ± 1%    4.10ms ± 1%  +11.82%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_ten[1d]),steps=100-16         10.5ms ± 0%    11.8ms ± 1%  +12.50%  (p=0.000 n=8+10)
RangeQuery/expr=changes(a_ten[1d]),steps=1000-16        77.6ms ± 1%    87.4ms ± 1%  +12.63%  (p=0.000 n=9+9)
RangeQuery/expr=changes(a_hundred[1d]),steps=1-16       30.4ms ± 2%    32.8ms ± 1%   +8.01%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_hundred[1d]),steps=10-16      37.1ms ± 2%    40.6ms ± 2%   +9.64%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_hundred[1d]),steps=100-16      105ms ± 1%     117ms ± 1%  +11.69%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_hundred[1d]),steps=1000-16     783ms ± 3%     876ms ± 1%  +11.83%  (p=0.000 n=9+10)
```

And then runtime v2.39 compared to after this commit:

```
name                                                  old time/op    new time/op    delta
RangeQuery/expr=changes(a_one[1d]),steps=1-16            391µs ± 2%     547µs ± 1%  +39.84%  (p=0.000 n=9+8)
RangeQuery/expr=changes(a_one[1d]),steps=10-16           452µs ± 2%     616µs ± 2%  +36.15%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_one[1d]),steps=100-16         1.12ms ± 1%    1.26ms ± 1%  +12.20%  (p=0.000 n=8+10)
RangeQuery/expr=changes(a_one[1d]),steps=1000-16        7.83ms ± 1%    7.95ms ± 1%   +1.59%  (p=0.000 n=10+8)
RangeQuery/expr=changes(a_ten[1d]),steps=1-16           2.98ms ± 0%    3.38ms ± 2%  +13.49%  (p=0.000 n=9+10)
RangeQuery/expr=changes(a_ten[1d]),steps=10-16          3.66ms ± 1%    4.02ms ± 1%   +9.80%  (p=0.000 n=10+9)
RangeQuery/expr=changes(a_ten[1d]),steps=100-16         10.5ms ± 0%    10.8ms ± 1%   +3.08%  (p=0.000 n=8+10)
RangeQuery/expr=changes(a_ten[1d]),steps=1000-16        77.6ms ± 1%    78.1ms ± 1%   +0.58%  (p=0.035 n=9+10)
RangeQuery/expr=changes(a_hundred[1d]),steps=1-16       30.4ms ± 2%    33.5ms ± 4%  +10.18%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_hundred[1d]),steps=10-16      37.1ms ± 2%    40.0ms ± 1%   +7.98%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_hundred[1d]),steps=100-16      105ms ± 1%     107ms ± 1%   +1.92%  (p=0.000 n=10+10)
RangeQuery/expr=changes(a_hundred[1d]),steps=1000-16     783ms ± 3%     775ms ± 1%   -1.02%  (p=0.019 n=9+9)
```

In summary, the runtime doesn't really improve with this change for
queries with just a few steps. For queries with many steps, this
commit essentially reinstates the old performance. This is good
because the many-step queries are the one that matter most (longest
absolute runtime).

In terms of allocations, though, this commit doesn't make a dent at
all (numbers not shown). The reason is that most of the allocations
happen in the sampleRingIterator (in the storage package), which has
to be addressed in a separate commit.

Signed-off-by: beorn7 <beorn@grafana.com>
2023-04-13 19:25:16 +02:00
Łukasz Mierzwa b6573353c1 Add query_samples_total metric
query_samples_total is a counter that tracks the total number of samples loaded by all queries.

The goal with this metric is to be able to see the amount of 'work' done by Prometheus to service queries.
At the moment we have metrics with the number of queries, plus more detailed metrics showing how much time each step of a query takes.
While those metrics do help they don't show us the whole picture.
Queries that do load more samples are (in general) more expensive than queries that do load fewer samples.
This means that looking only at the number of queries doesn't tell us how much 'work' Prometheus received.
Adding a counter that tracks the total number of samples loaded allows us to see if there was a spike in the cost of queries, not just the number of them.

Signed-off-by: Łukasz Mierzwa <l.mierzwa@gmail.com>
2023-04-12 14:05:06 +01:00
Ganesh Vernekar 5588cab8b2
Merge pull request #12173 from bboreham/builder-no-empty-labels
labels: simplify call to get Labels from Builder
2023-04-04 12:02:55 +05:30