New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WithQueryStats option #1035
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -421,10 +421,36 @@ type Metadata struct { | |
Unit string `json:"unit"` | ||
} | ||
|
||
type stepStat struct { | ||
T int64 | ||
V float64 | ||
} | ||
|
||
type queryTimings struct { | ||
EvalTotalTime float64 `json:"evalTotalTime"` | ||
ResultSortTime float64 `json:"resultSortTime"` | ||
QueryPreparationTime float64 `json:"queryPreparationTime"` | ||
InnerEvalTime float64 `json:"innerEvalTime"` | ||
ExecQueueTime float64 `json:"execQueueTime"` | ||
ExecTotalTime float64 `json:"execTotalTime"` | ||
} | ||
|
||
type querySamples struct { | ||
TotalQueryableSamplesPerStep []stepStat `json:"totalQueryableSamplesPerStep,omitempty"` | ||
TotalQueryableSamples int `json:"totalQueryableSamples"` | ||
PeakSamples int `json:"peakSamples"` | ||
} | ||
|
||
type QueryStats struct { | ||
Timings queryTimings `json:"timings"` | ||
Samples querySamples `json:"samples"` | ||
} | ||
|
||
// queryResult contains result data for a query. | ||
type queryResult struct { | ||
Type model.ValueType `json:"resultType"` | ||
Result interface{} `json:"result"` | ||
Stats *QueryStats `json:"stats"` | ||
|
||
// The decoded value. | ||
v model.Value | ||
|
@@ -580,7 +606,10 @@ func (qr *queryResult) UnmarshalJSON(b []byte) error { | |
v := struct { | ||
Type model.ValueType `json:"resultType"` | ||
Result json.RawMessage `json:"result"` | ||
}{} | ||
Stats *QueryStats `json:"stats"` | ||
}{ | ||
Stats: qr.Stats, | ||
} | ||
|
||
err := json.Unmarshal(b, &v) | ||
if err != nil { | ||
|
@@ -819,7 +848,9 @@ func (h *httpAPI) LabelValues(ctx context.Context, label string, matches []strin | |
} | ||
|
||
type apiOptions struct { | ||
timeout time.Duration | ||
timeout time.Duration | ||
stats *QueryStats | ||
perStepStats bool | ||
} | ||
|
||
type Option func(c *apiOptions) | ||
|
@@ -830,6 +861,13 @@ func WithTimeout(timeout time.Duration) Option { | |
} | ||
} | ||
|
||
func WithQueryStats(s *QueryStats, perStep bool) Option { | ||
return func(o *apiOptions) { | ||
o.stats = s | ||
o.perStepStats = perStep | ||
} | ||
} | ||
|
||
func (h *httpAPI) Query(ctx context.Context, query string, ts time.Time, opts ...Option) (model.Value, Warnings, error) { | ||
|
||
u := h.client.URL(epQuery, nil) | ||
|
@@ -845,6 +883,10 @@ func (h *httpAPI) Query(ctx context.Context, query string, ts time.Time, opts .. | |
q.Set("timeout", d.String()) | ||
} | ||
|
||
if opt.perStepStats { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be nicer to stick to some enum like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's mostly boolean because I based this on the PR for Prometheus that adds it, and it had a bool - https://github.com/prometheus/prometheus/blob/ce6a643ee88fba7c02fbd0459c4d0ac498f512dd/promql/engine.go#L130 The only supported value here is "all", at least at the moment. IMHO ideally it should be an enum in Prometheus source, since that's where it's consumed and used, but I imagine we want to avoid prometheus & client_golang circular dependencies, so it seems like we'll need to duplicate supported values in client_golang. Side note: I do find it awkward at time to use client_golang to interact with Prometheus, it's easy for client_golang to fall behind Prometheus (structs for JSON might not get updated, #925) so I wonder if in the future it would make sense to move API client libs to Prometheus repo itself, especially now that it's versioned better and so it's easier to import and use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well there is problem - you don't always want to pull the Prometheus repo. But multi-module repo is also some idea (: |
||
q.Set("stats", "all") | ||
} | ||
|
||
q.Set("query", query) | ||
if !ts.IsZero() { | ||
q.Set("time", formatTime(ts)) | ||
|
@@ -856,6 +898,10 @@ func (h *httpAPI) Query(ctx context.Context, query string, ts time.Time, opts .. | |
} | ||
|
||
var qres queryResult | ||
if opt.stats != nil { | ||
qres.Stats = opt.stats | ||
} | ||
|
||
return model.Value(qres.v), warnings, json.Unmarshal(body, &qres) | ||
} | ||
|
||
|
@@ -878,12 +924,19 @@ func (h *httpAPI) QueryRange(ctx context.Context, query string, r Range, opts .. | |
q.Set("timeout", d.String()) | ||
} | ||
|
||
if opt.perStepStats { | ||
q.Set("stats", "all") | ||
} | ||
|
||
_, body, warnings, err := h.client.DoGetFallback(ctx, u, q) | ||
if err != nil { | ||
return nil, warnings, err | ||
} | ||
|
||
var qres queryResult | ||
if opt.stats != nil { | ||
qres.Stats = opt.stats | ||
} | ||
|
||
return model.Value(qres.v), warnings, json.Unmarshal(body, &qres) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bit unexpected as our method will return value via variadic option (: So let's add a comment at least.
But I would be curious if we can actually change
model.Value
with some interface that is like model.Value but also returns stats 🤔 Will be much nicer for users.And this package is in experimental stage so we can change API btw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant, I'll try to come up with an API change that makes it easier to consume this.