Sample stats: total and peak #10369

aughr · 2022-02-28T01:31:22Z

This PR covers most of what we proposed in #10181. It's a combination of my work, @alanprot's work, and @Harkishen-Singh's work.

It adds new stats:

totalQueryableSamples: The total number of samples read out of the underlying Queryable instance inclusive of any samples that are buffered between range steps.
totalQueryableSamplesPerStep: The above statistic tracked per step in a range query. These will always add up to the value in totalQueryableSamples. The engine does not compute or report these by default. We only do the work and report the data if an engine flag is set (available via --enable-feature per-step-stats from the CLI) and a per-query flag is set (stats=all in the request). We introduce a new QueryOpts struct in the Engine's API to make having these kinds of per-query options easier in the future.
peakSamples: The high water mark of the value compared against MaxSamples during the course of query execution.

Per @bboreham's feedback, this PR also changes the query stats response from a struct to an interface and adds a way to override that response. Implementations like Cortex can use this to read stats from the Context and insert them into the response that's sent to the user and/or remove information from the stats response.

This does not include the series stats we proposed because we have some more thinking to do about how best to represent them. More to come on that front.

The performance impact of the new statistics that are always on should be negligible per our local benchmarking. Even the per-step statistics don't have much impact outside the combination of very simple queries with many steps. I'll upload these benchmark runs in a bit.

An example JSON response with all these stats on for max_over_time(up[5m]) with 30-second steps over 5 minutes. The underlying data here started only a minute before running the query.

{
  "status": "success",
  "data": {
    "resultType": "matrix",
    "result": [
      {
        "metric": {
          "instance": "localhost:9090",
          "job": "prometheus"
        },
        "values": [
          [
            1645813076,
            "1"
          ],
          [
            1645813106,
            "1"
          ],
          [
            1645813136,
            "1"
          ],
          [
            1645813166,
            "1"
          ],
          [
            1645813196,
            "1"
          ],
          [
            1645813226,
            "1"
          ],
          [
            1645813256,
            "1"
          ],
          [
            1645813286,
            "1"
          ],
          [
            1645813316,
            "1"
          ],
          [
            1645813346,
            "1"
          ],
          [
            1645813376,
            "1"
          ]
        ]
      }
    ],
    "stats": {
      "timings": {
        "evalTotalTime": 0.006922879,
        "resultSortTime": 2.677e-06,
        "queryPreparationTime": 0.003402187,
        "innerEvalTime": 0.002431834,
        "execQueueTime": 0.000819559,
        "execTotalTime": 0.008257353
      },
      "samples": {
        "totalQueryableSamplesPerStep": [
          [
            1645813076,
            "4"
          ],
          [
            1645813106,
            "6"
          ],
          [
            1645813136,
            "8"
          ],
          [
            1645813166,
            "10"
          ],
          [
            1645813196,
            "12"
          ],
          [
            1645813226,
            "14"
          ],
          [
            1645813256,
            "16"
          ],
          [
            1645813286,
            "18"
          ],
          [
            1645813316,
            "20"
          ],
          [
            1645813346,
            "20"
          ],
          [
            1645813376,
            "20"
          ]
        ],
        "totalQueryableSamples": 148,
        "peakSamples": 31
      }
    }
  }
}

beorn7 · 2022-03-03T18:09:00Z

/prombench edfe657

prombot · 2022-03-03T18:09:00Z

Incorrect prombench syntax, please find correct syntax here.

beorn7 · 2022-03-03T18:10:05Z

/prombench main

prombot · 2022-03-03T18:10:06Z

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-10369 and main

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

roidelapluie

Thanks, in general, this LGTM.

Do we need to compute the peak and total? Could we let that to the caller instead?

roidelapluie · 2022-03-04T21:11:17Z

util/stats/query_stats.go

+
+type Stats struct {
+	TimerStats  *QueryTimers
+	SampleStats *QuerySamples


If we are concerned about impact, we could later on make that an interface, so when it is disabled we have a NoopSampleStats that has lesser impact. I see @beorn7 has launched prombench already, it might not be needed.

cmd/prometheus/main.go

roidelapluie · 2022-03-04T21:12:13Z

promql/engine.go

@@ -379,12 +394,12 @@ func (ng *Engine) SetQueryLogger(l QueryLogger) {
 }

 // NewInstantQuery returns an evaluation query for the given expression at the given time.
-func (ng *Engine) NewInstantQuery(q storage.Queryable, qs string, ts time.Time) (Query, error) {
+func (ng *Engine) NewInstantQuery(q storage.Queryable, opts QueryOpts, qs string, ts time.Time) (Query, error) {


I really like this, because we want to jave later on per-query lookback delta, it could be a query opt as well.

Can we have QueryOpts as a pointer ? we are passing that downstream mutliple times, creating un-needed allocs. it would also be nicer in tests.

prombot · 2022-03-06T18:15:36Z

Benchmark tests are running for 3 days! If this is intended ignore this message otherwise you can cancel it by commenting: /prombench cancel

beorn7 · 2022-03-07T12:53:20Z

/prombench cancel

I assume you have seen enough benchmark data.

prombot · 2022-03-07T12:53:21Z

Benchmark cancel is in progress.

alanprot · 2022-03-07T16:40:28Z

Hi @roidelapluie, thanks a lot for looking into this.

Do we need to compute the peak and total? Could we let that to the caller instead?

I don't think is possible to compute the one stats from another as they are tracking different things (Peak is not the max(totalQueryableSamples) but instead is the max samples kept in memory during the query execution. The Peak stats show how close the query was to reach the query.max-samples limit while the totalQueryableSamples shows how many samples was used to run the query.

roidelapluie · 2022-03-08T12:16:13Z

Hi @roidelapluie, thanks a lot for looking into this.

Do we need to compute the peak and total? Could we let that to the caller instead?

I don't think is possible to compute the one stats from another as they are tracking different things (Peak is not the max(totalQueryableSamples) but instead is the max samples kept in memory during the query execution. The Peak stats show how close the query was to reach the query.max-samples limit while the totalQueryableSamples shows how many samples was used to run the query.

I mean, can we only track them per step, and not compute the overall maximum.

aughr · 2022-03-08T15:48:44Z

I mean, can we only track them per step, and not compute the overall maximum.

We're currently not recording peak samples per step, just the total samples queried. I think it's doable, so if you'd like to see it in this PR, I can take a shot at it. That said, because per-step stats are disabled by default, while overall stats are always on, I'd prefer to always compute overall stats.

aughr · 2022-03-11T19:02:17Z

One thing I've been thinking about doing before landing this is changing the stats suffix from PerTime to PerStep. Thoughts @roidelapluie and others?

roidelapluie · 2022-03-15T23:24:55Z

👍 for PerStep

roidelapluie · 2022-03-18T23:45:36Z

close/reopen to trigger circleci

roidelapluie

Thanks! I have got the time to review & test the pull request.

Somehow,

"samples": {
                "peakSamples": 153,
                "totalQueryableSamples": 126
            },

are always reported. I think it's fine, we could even later add those fields to the UI.

In my review, I noted a wrong change in tsdb, and comments that do not end with full stop. The feature flag description is also incorrect.

Thanks!

cmd/prometheus/main.go

promql/engine.go

promql/engine_test.go

tsdb/head_test.go

roidelapluie · 2022-03-19T00:22:08Z

Windows tests should be fixed by rebasing on top of main.

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 allows other implementations to inject their own statistics that they're gathering in data linked from the context.Context. For example, Cortex can inject its stats.Stats value under the `cortex` key. Signed-off-by: Andrew Bloomgarden <blmgrdn@amazon.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>

roidelapluie · 2022-03-21T13:58:45Z

triggering ci

roidelapluie · 2022-03-21T22:49:23Z

Thanks!

Per Julien's feedback on prometheus#10369, we're choosing to be consistent with data types inside the stats structure (ints) rather than with the points format that is part of the normal query responses (strings). We have this option because this data cannot be NaN/Inf. Signed-off-by: Andrew Bloomgarden <blmgrdn@amazon.com>

Per Julien's feedback on #10369, we're choosing to be consistent with data types inside the stats structure (ints) rather than with the points format that is part of the normal query responses (strings). We have this option because this data cannot be NaN/Inf. Signed-off-by: Andrew Bloomgarden <blmgrdn@amazon.com>

roidelapluie · 2022-04-13T14:25:09Z

/prombench v2.34.0

prombot · 2022-04-13T14:25:11Z

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-10369 and v2.34.0

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart v2.34.0

roidelapluie · 2022-04-14T12:51:25Z

/prombench cancel

prombot · 2022-04-14T12:51:26Z

Benchmark cancel is in progress.

roidelapluie · 2022-04-14T12:53:30Z

This pull request has memory issues but it's head has been rebased on top of #10450, which I believe to be the actual issue.

prometheus/prometheus#10369 was part of v2.35.0 release and it exposes more stats. There's currently no way to access those stats since Query() and RangeQuery() returns only a subset of Prometheus API response. To avoid bloating Query() return values I'm adding this as a new Option where you can pass a pointer to a QueryStats struct that will hold all recorded stats, instead of adding another return value. Signed-off-by: Lukasz Mierzwa <l.mierzwa@gmail.com>

aughr requested review from JessicaGreben, dgl, codesome and roidelapluie as code owners February 28, 2022 01:31

prombot added the prombench label Mar 3, 2022

roidelapluie reviewed Mar 4, 2022

View reviewed changes

aughr force-pushed the sample_stats branch from 3ea99bf to 9f5f3ed Compare March 17, 2022 15:38

roidelapluie closed this Mar 18, 2022

roidelapluie reopened this Mar 18, 2022

roidelapluie reviewed Mar 19, 2022

View reviewed changes

Harkishen-Singh mentioned this pull request Mar 21, 2022

Adds support for involved samples for query stats #6890

Closed

aughr force-pushed the sample_stats branch from 7d48323 to b9cc6c0 Compare March 21, 2022 13:56

alanprot and others added 3 commits March 21, 2022 09:57

aughr force-pushed the sample_stats branch from b9cc6c0 to 7de5670 Compare March 21, 2022 13:57

roidelapluie closed this Mar 21, 2022

roidelapluie reopened this Mar 21, 2022

roidelapluie approved these changes Mar 21, 2022

View reviewed changes

roidelapluie merged commit a64b9fe into prometheus:main Mar 21, 2022

aughr mentioned this pull request Mar 23, 2022

Switch per-step stats to serialize ints #10479

Merged

This was referenced Apr 6, 2022

Query Sample Statistics cortexproject/cortex#4708

Merged

Changing TotalQueryableSamples from int to int64 #10549

Merged

roidelapluie mentioned this pull request Apr 7, 2022

cut v2.35.0-rc0 #10543

Merged

prymitive mentioned this pull request Apr 22, 2022

Add WithQueryStats option prometheus/client_golang#1035

Closed

Sample stats: total and peak #10369

Sample stats: total and peak #10369

aughr commented Feb 28, 2022 •

edited

beorn7 commented Mar 3, 2022

prombot commented Mar 3, 2022

beorn7 commented Mar 3, 2022

prombot commented Mar 3, 2022

roidelapluie left a comment

roidelapluie Mar 4, 2022

roidelapluie Mar 4, 2022

roidelapluie Mar 15, 2022

prombot commented Mar 6, 2022

beorn7 commented Mar 7, 2022

prombot commented Mar 7, 2022

alanprot commented Mar 7, 2022 •

edited

roidelapluie commented Mar 8, 2022

aughr commented Mar 8, 2022

aughr commented Mar 11, 2022

roidelapluie commented Mar 15, 2022

roidelapluie commented Mar 18, 2022

roidelapluie left a comment

roidelapluie commented Mar 19, 2022

roidelapluie commented Mar 21, 2022

roidelapluie commented Mar 21, 2022

roidelapluie commented Apr 13, 2022

prombot commented Apr 13, 2022

roidelapluie commented Apr 14, 2022

prombot commented Apr 14, 2022

roidelapluie commented Apr 14, 2022

Sample stats: total and peak #10369

Sample stats: total and peak #10369

Conversation

aughr commented Feb 28, 2022 • edited

beorn7 commented Mar 3, 2022

prombot commented Mar 3, 2022

beorn7 commented Mar 3, 2022

prombot commented Mar 3, 2022

roidelapluie left a comment

Choose a reason for hiding this comment

roidelapluie Mar 4, 2022

Choose a reason for hiding this comment

roidelapluie Mar 4, 2022

Choose a reason for hiding this comment

roidelapluie Mar 15, 2022

Choose a reason for hiding this comment

prombot commented Mar 6, 2022

beorn7 commented Mar 7, 2022

prombot commented Mar 7, 2022

alanprot commented Mar 7, 2022 • edited

roidelapluie commented Mar 8, 2022

aughr commented Mar 8, 2022

aughr commented Mar 11, 2022

roidelapluie commented Mar 15, 2022

roidelapluie commented Mar 18, 2022

roidelapluie left a comment

Choose a reason for hiding this comment

roidelapluie commented Mar 19, 2022

roidelapluie commented Mar 21, 2022

roidelapluie commented Mar 21, 2022

roidelapluie commented Apr 13, 2022

prombot commented Apr 13, 2022

roidelapluie commented Apr 14, 2022

prombot commented Apr 14, 2022

roidelapluie commented Apr 14, 2022

aughr commented Feb 28, 2022 •

edited

alanprot commented Mar 7, 2022 •

edited