Add WithQueryStats option #1035
Conversation
prometheus/prometheus#10369 was part of v2.35.0 release and it exposes more stats. There's currently no way to access those stats since Query() and RangeQuery() returns only a subset of Prometheus API response. To avoid bloating Query() return values I'm adding this as a new Option where you can pass a pointer to a QueryStats struct that will hold all recorded stats, instead of adding another return value. Signed-off-by: Lukasz Mierzwa <l.mierzwa@gmail.com>
| @@ -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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (:
| @@ -830,6 +861,13 @@ func WithTimeout(timeout time.Duration) Option { | |||
| } | |||
| } | |||
|
|
|||
| func WithQueryStats(s *QueryStats, perStep bool) Option { | |||
There was a problem hiding this comment.
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.
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.
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
prometheus/prometheus#10369 was part of v2.35.0 release and it exposes more stats.
There's currently no way to access those stats since Query() and RangeQuery() returns only a subset of Prometheus API response.
To avoid bloating Query() return values I'm adding this as a new Option where you can pass a pointer to a QueryStats struct that will hold all recorded stats,
instead of adding another return value.
Signed-off-by: Lukasz Mierzwa l.mierzwa@gmail.com