Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 55 additions & 2 deletions api/prometheus/v1/api.go
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -830,6 +861,13 @@ func WithTimeout(timeout time.Duration) Option {
}
}

func WithQueryStats(s *QueryStats, perStep bool) Option {
Copy link
Member

@bwplotka bwplotka Apr 23, 2022

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this package is in experimental stage so we can change API btw.

Brilliant, I'll try to come up with an API change that makes it easier to consume this.

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)
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nicer to stick to some enum like all and minimal instead of opt.perStepStats bool - we will be resilient to adding any other option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
https://github.com/prometheus/prometheus/blob/a64b9fe323cf913c212c1829fe93d1a9dbc63348/web/api/v1/api.go#L433
Not sure if it's worth to create a client abstraction for it but happy to do so.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Well there is problem - you don't always want to pull the Prometheus repo.
So the idea was to do this: #896

But multi-module repo is also some idea (:

q.Set("stats", "all")
}

q.Set("query", query)
if !ts.IsZero() {
q.Set("time", formatTime(ts))
Expand All @@ -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)
}

Expand All @@ -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)
}
Expand Down
22 changes: 22 additions & 0 deletions api/prometheus/v1/api_test.go
Expand Up @@ -1300,6 +1300,28 @@ func TestAPIs(t *testing.T) {
},
},
},
{
do: doQuery("2", testTime, WithQueryStats(nil, true)),
inRes: &queryResult{
Type: model.ValScalar,
Result: &model.Scalar{
Value: 2,
Timestamp: model.TimeFromUnix(testTime.Unix()),
},
},

reqMethod: "POST",
reqPath: "/api/v1/query",
reqParam: url.Values{
"query": []string{"2"},
"time": []string{testTime.Format(time.RFC3339Nano)},
"stats": []string{"all"},
},
res: &model.Scalar{
Value: 2,
Timestamp: model.TimeFromUnix(testTime.Unix()),
},
},
}

var tests []apiTest
Expand Down
27 changes: 27 additions & 0 deletions api/prometheus/v1/example_test.go
Expand Up @@ -218,3 +218,30 @@ func ExampleAPI_series() {
fmt.Println(lbl)
}
}

func ExampleAPI_queryWithStats() {
client, err := api.NewClient(api.Config{
Address: "http://demo.robustperception.io:9090",
})
if err != nil {
fmt.Printf("Error creating client: %v\n", err)
os.Exit(1)
}

v1api := v1.NewAPI(client)
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
stats := v1.QueryStats{}
result, warnings, err := v1api.Query(ctx, "up", time.Now(),
v1.WithTimeout(5*time.Second),
v1.WithQueryStats(&stats, true))
if err != nil {
fmt.Printf("Error querying Prometheus: %v\n", err)
os.Exit(1)
}
if len(warnings) > 0 {
fmt.Printf("Warnings: %v\n", warnings)
}
fmt.Printf("Result:\n%v\n", result)
fmt.Printf("Stats:\n%v\n", stats)
}