Add new query stats metrics to track prometheus querystats #6228
Conversation
SungJin1212
force-pushed
the
Add-metrics-for-queryable-samples-and-peak-samples
branch
4 times, most recently
from
0a571de
to
d4ddae7
Compare
pkg/frontend/transport/handler.go
Outdated
|
|
||
| h.queryPeakSamples = promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ | ||
| Name: "cortex_query_peak_samples_total", | ||
| Help: "Highest count of samples considered to execute a query.", |
There was a problem hiding this comment.
Can you please explain the usecase of tracking peak samples using a counter metric?
There was a problem hiding this comment.
I think like promQL: sum by (user) (irate(cortex_query_peak_samples_total[5m])), the cortex user can track the rate of how many samples are loaded in memory for each tenant's overall queries.
One usecase is:
- The cortex user can track query memory usage (used by loaded samples) for each tenant and turn promQL to reduce query memory usage.
There was a problem hiding this comment.
OK. For a counter metric I feel it is more useful to just track total samples, not peak samples.
Would it be more useful to use native histogram instead?
There was a problem hiding this comment.
Yes, I think the histogram is the best.
pkg/frontend/transport/handler.go
Outdated
| @@ -122,6 +124,16 @@ func NewHandler(cfg HandlerConfig, roundTripper http.RoundTripper, log log.Logge | |||
| Help: "Number of samples fetched to execute a query.", | |||
| }, []string{"user"}) | |||
|
|
|||
| h.queryTotalQueryableSamples = promauto.With(reg).NewCounterVec(prometheus.CounterOpts{ | |||
| Name: "cortex_query_total_queryable_samples_total", | |||
| Help: "Number of total queryable samples to execute a query.", | |||
There was a problem hiding this comment.
I feel queryable samples is a bit confusing.
What about cortex_query_samples_processed_total or cortex_query_samples_scanned_total?
You can add a comment to reference it to https://github.com/prometheus/prometheus/blob/main/util/stats/query_stats.go#L237
There was a problem hiding this comment.
For me, cortex_query_samples_processed_total is more good. Thank you!
There was a problem hiding this comment.
How about changing the variable name queryTotalQueryableSamples to queryProcessedSamples and querySamples to queryFetchedSamples?
There was a problem hiding this comment.
I think we can keep queryTotalQueryableSamples as it is clear that it refers to Total Samples.
Makes sense to rename querySamples to queryFetchedSamples.
There was a problem hiding this comment.
I have fixed it.
SungJin1212
force-pushed
the
Add-metrics-for-queryable-samples-and-peak-samples
branch
3 times, most recently
from
afea4c7
to
1e37e70
Compare
|
@yeya24 |
SungJin1212
force-pushed
the
Add-metrics-for-queryable-samples-and-peak-samples
branch
from
1e37e70
to
0cfa84b
Compare
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
SungJin1212
force-pushed
the
Add-metrics-for-queryable-samples-and-peak-samples
branch
from
0cfa84b
to
797b6ee
Compare
What this PR does:
Add new query stats metrics
cortex_query_total_queryable_samples_totalandcortex_query_peak_samples.These track
TotalQueryableSamplesandPeakSamplesfor each in https://github.com/prometheus/prometheus/blob/main/util/stats/query_stats.goWhich issue(s) this PR fixes:
Fixes
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]