New metric for measuring query duration proposal #4932
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: mzardab <[email protected]>
Signed-off-by: mzardab <[email protected]>
Signed-off-by: mzardab <[email protected]>
Signed-off-by: mzardab <[email protected]>
|
That's a really nice improvement on query latency observability, especially with the new query pushdown feature 👍 |
bwplotka
reviewed
Dec 14, 2021
There was a problem hiding this comment.
Great proposal, but as I mentioned offline, it would be awesome to take this latency number right when we return query PromQL response to the user.
This might have to be changed if we try to push query eval to Query Frontent, but no clear proposal for now so let's go for this!
docs/proposals-accepted/202108-more-granular-query-performance-metrics.md
Show resolved
Hide resolved
docs/proposals-accepted/202108-more-granular-query-performance-metrics.md
Show resolved
Hide resolved
docs/proposals-accepted/202108-more-granular-query-performance-metrics.md
Show resolved
Hide resolved
docs/proposals-accepted/202108-more-granular-query-performance-metrics.md
Show resolved
Hide resolved
moadz
changed the title
moadz
commented
Dec 22, 2021
docs/proposals-accepted/202108-more-granular-query-performance-metrics.md
Outdated
Show resolved
Hide resolved
moadz
changed the title
moadz
commented
Dec 23, 2021
Comment on lines
+258
to
+265
| * Amend the `seriesServer` to keep track of all `SeriesStats` for each series pushed to it | ||
| * Amend the static `qapi.queryableCreate` to take a `SeriesStatsReporter` func parameter that will exfiltrate the seriesStats from the Thanos Proxy StoreAPI | ||
| * Add new runtime flags that will allow us to specify a) Query time quantiles b) Series size quantiles c) Sample size quantiles for our partitioned histogram | ||
| * Start a query duration timer as soon as the handler is hit | ||
| * Create a new partitioned vector histogram called `thanos_query_duration_seconds` in the `queryRange` API handler | ||
| * Propagate all exfiltrated `SeriesStats` to aforementioned metric | ||
| * Record observations against the `thanos_query_duration_seconds` histogram after bucketing samples_le/series_le buckets | ||
|
|
There was a problem hiding this comment.
@bwplotka amended to include the entire query path (with stats exfiltration)
bwplotka
reviewed
Dec 23, 2021
| * Do not want to create separate histograms for each individual store query, so they will need to be aggregated at the `Series` request level so that our observations include all | ||
| * Do not want to block series receive on a `seriesStats` merging mutex for each incoming response, so maintaining a central `seriesStats` reference and passing it into each of the proxied store requests is out of the question | ||
|
|
||
| ### Why can't we capture the query shape & latency spanning the entire query path? |
bwplotka
reviewed
Dec 23, 2021
|
|
||
| **tl;dr:** Longer term to capture the entire query path by amending the Prometheus Querier API to return some stats alongside the query, and creating this generic metric inside the Prometheus PromQL engine. Short term, pass a func parameter to the Queryable constructor for the proxy StoreAPI querier that will exfiltrate the `SeriesStats`, circumventing PromQL engine. | ||
|
|
||
| ### Measuring Thanos Query Latency with respect to query fanout |
There was a problem hiding this comment.
Suggested change
| ### Measuring Thanos Query Latency with respect to query fanout | |
| ### Measuring Thanos Query Latency with respect to query fanout | |
bwplotka
approved these changes
Dec 23, 2021
|
Looks perfect. Only two flakes on CI, so merging, unless there are objections from others 👍🏽 |
|
Documentation check was not passed so that the following PRs will be failed. @moadz Could you help to fix the Documentation check issue? Thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Moad Zardab [email protected]
Related to #4895