Add query metrics to ruler query stats #6173
Conversation
SungJin1212
force-pushed
the
Add-query-metrics-to-ruler-query-stats
branch
2 times, most recently
from
0f39329 to
2ef5183
Compare
SungJin1212
force-pushed
the
Add-query-metrics-to-ruler-query-stats
branch
2 times, most recently
from
dbb3cca to
f0d770d
Compare
pkg/ruler/compat.go
Outdated
| queryTime := evalMetrics.RulerQuerySeconds.WithLabelValues(userID) | ||
| querySeries := evalMetrics.RulerQuerySeries.WithLabelValues(userID) | ||
| querySample := evalMetrics.RulerQuerySamples.WithLabelValues(userID) | ||
| queryChunkBytes := evalMetrics.RulerQuerySamples.WithLabelValues(userID) | ||
| queryDataBytes := evalMetrics.RulerQueryDataBytes.WithLabelValues(userID) | ||
| queryFunc = RecordAndReportRuleQueryMetrics(metricsQueryFunc, queryTime, querySeries, querySample, queryChunkBytes, queryDataBytes, logger) |
There was a problem hiding this comment.
Nit: I think you can pass just evalMetrics as parameter to RecordAndReportRuleQueryMetrics and to all initialization inside it.
| queryTime := evalMetrics.RulerQuerySeconds.WithLabelValues(userID) | |
| querySeries := evalMetrics.RulerQuerySeries.WithLabelValues(userID) | |
| querySample := evalMetrics.RulerQuerySamples.WithLabelValues(userID) | |
| queryChunkBytes := evalMetrics.RulerQuerySamples.WithLabelValues(userID) | |
| queryDataBytes := evalMetrics.RulerQueryDataBytes.WithLabelValues(userID) | |
| queryFunc = RecordAndReportRuleQueryMetrics(metricsQueryFunc, queryTime, querySeries, querySample, queryChunkBytes, queryDataBytes, logger) | |
| queryFunc = RecordAndReportRuleQueryMetrics(metricsQueryFunc, userID, evalMetrics) |
This way if new query stats metrics are added in the future, we don't need to change function signatures.
There was a problem hiding this comment.
However this will not be consistent with the other functions, such as MetricsQueryFunc and NewPusherAppandable, therefore I think it is ok to ignore my nit, unless anyone wants to chime in.
|
Overall this change looks good to me. It is a useful addition to the project and will help troubleshoot issues faster. |
SungJin1212
force-pushed
the
Add-query-metrics-to-ruler-query-stats
branch
from
f0d770d to
ed9acee
Compare
|
@rapphil |
|
@alanprot |
pkg/ruler/compat.go
Outdated
| queryTime := evalMetrics.RulerQuerySeconds.WithLabelValues(userID) | ||
| querySeries := evalMetrics.RulerQuerySeries.WithLabelValues(userID) | ||
| querySample := evalMetrics.RulerQuerySamples.WithLabelValues(userID) | ||
| queryChunkBytes := evalMetrics.RulerQueryChunkBytes.WithLabelValues(userID) | ||
| queryDataBytes := evalMetrics.RulerQueryDataBytes.WithLabelValues(userID) |
There was a problem hiding this comment.
these should be outside of the scope of this anonymous function. they should be in the scope of RecordAndReportRuleQueryMetrics.
There was a problem hiding this comment.
I have fixed it. Thank you.
SungJin1212
force-pushed
the
Add-query-metrics-to-ruler-query-stats
branch
from
ed9acee to
9a75d4c
Compare
|
LGTM. Thanks for this valuable contribution |
|
Nice.. this LGTM as well. |
|
@alanprot |
CHANGELOG.md
Outdated
| @@ -54,6 +54,7 @@ | |||
| * [ENHANCEMENT] Compactor: Introduce cleaner visit marker. #6113 | |||
| * [ENHANCEMENT] Query Frontend: Add cortex_query_samples_total metric. #6142 | |||
| * [ENHANCEMENT] Ingester: Implement metadata API limit. #6128 | |||
| * [ENHANCEMENT] Ruler: Add query statistics metrics when --ruler.query-stats-enabled=true. #6173 | |||
There was a problem hiding this comment.
We need to fix changelog. Can we move this to master section?
Signed-off-by: SungJin1212 <[email protected]>
SungJin1212
force-pushed
the
Add-query-metrics-to-ruler-query-stats
branch
from
9a75d4c to
b75be24
Compare
What this PR does:
Add query statistics metrics to Ruler when --ruler.query-stats-enabled=true to track how many data are fetched by the alerting rule or recording rules for each tenants.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]