Add PromQL query and query_range support #124
Conversation
By using the Prometheus QueryEngine and some utility functions from the Querier implementation for remote storage we enable Timescale-Prometheus to return results for query and query_range PromQL requests. Implementation is rudimentary, only direct requests to these two endpoints can be executed. Needs implementation of the methods that return metadata.
| @@ -64,7 +68,8 @@ type MetricCache interface { | |||
| } | |||
|
|
|||
| type pgxConnImpl struct { | |||
| conn *pgxpool.Pool | |||
| conn *pgxpool.Pool | |||
| readHist prometheus.ObserverVec | |||
There was a problem hiding this comment.
Since this only seems to be needed on the Query side, can we add it to some reader-only state instead?
| @@ -677,19 +690,64 @@ func (q *pgxQuerier) HealthCheck() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (q *pgxQuerier) Select(mint int64, maxt int64, sortSeries bool, hints *storage.SelectHints, ms ...*labels.Matcher) (storage.SeriesSet, storage.Warnings, error) { | |||
There was a problem hiding this comment.
Sinve all of this stuff seems to be non-pgx specific, should it go in querier.go, or another file, instead?
There was a problem hiding this comment.
++ but probably not for this PR (It also requires moving Query function below and maybe some other things; This PR is already big). I'd add a todo on the project board or an issue.
pkg/pgmodel/series_set.go
Outdated
| return found | ||
| } | ||
|
|
||
| func (p *pgxSeriesIterator) getCurrTs() int64 { |
There was a problem hiding this comment.
should this return a model.Time?
(btw, I have a version with what I believe to be correct overflow handling here, not sure how much we care)
There was a problem hiding this comment.
implements a prometheus defined interface
| @@ -182,6 +153,12 @@ func (c *clauseBuilder) build() ([]string, []interface{}) { | |||
| return c.clauses, c.args | |||
| } | |||
|
|
|||
| func buildSeriesSet(rows []pgx.Rows, sortSeries bool) (storage.SeriesSet, storage.Warnings, error) { | |||
| return &pgxSeriesSet{ | |||
| rows: rows, | |||
There was a problem hiding this comment.
I'm slightly tempted to say we should just write our own type with our own DecodeBinary, and return the query results directly as the iterator, but I'm not sure if it's worth.
|
I also don't like how the decoding code is using arbitrary-seeming array-indexes everywhere; at minimum I'd like documentation for expected layout, and ideally I'd like pgx to error if the postgres types don't match the type we expect |
There was a problem hiding this comment.
Great work. I think the thing that need the most changing is the series set stuff. The code right now is pretty complex and doesn't handle nulls. The advantages of the approach are unclear because of the need to copy the byte slice anyway. I suggested a slightly different approach that may not be quite as efficient but I think is cleaner for now until we get some profiling data.
| @@ -0,0 +1,145 @@ | |||
| package api | |||
There was a problem hiding this comment.
Should we make issues/todos for creating query_test and query_range tests just like the other endpoints? Not to be implemented for this PR.
| @@ -677,19 +690,64 @@ func (q *pgxQuerier) HealthCheck() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (q *pgxQuerier) Select(mint int64, maxt int64, sortSeries bool, hints *storage.SelectHints, ms ...*labels.Matcher) (storage.SeriesSet, storage.Warnings, error) { | |||
There was a problem hiding this comment.
++ but probably not for this PR (It also requires moving Query function below and maybe some other things; This PR is already big). I'd add a todo on the project board or an issue.
|
This also needs a rebase |
atanasovskib
force-pushed
the
add-promql-endpoint
branch
from
35fb905
to
1d8e035
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
antekresic
force-pushed
the
add-promql-endpoint
branch
from
1d8e035
to
b5697b2
Compare
|
@cevian @JLockerman I've updated the PR with your suggestion. I didn't include the overflow checks as suggested but we really care, can add those as well. Can you take another look? Thanks! |
There was a problem hiding this comment.
Review of the series_set stuff only. @antekresic looks much better. Have a few change requests with regard to proper NULL handling.
antekresic
force-pushed
the
add-promql-endpoint
branch
from
b5697b2
to
a93a62e
Compare
|
further I think this code may be a problem; if I'm reading it right it'll send the queries one-at-a-time to the database, and incur latency proportional to the number of metrics, if we batched them we would only have 1 round-trip-time. Additionally, if we batched them that function could return a |
|
(that last bit should probably wait for the next PR) |
In order to reduce the performance impact of transforming the data coming from the database in multiple formats, this commits adds the ability to read the values from binary data as its needed by the query engine.
antekresic
force-pushed
the
add-promql-endpoint
branch
from
7a4033e
to
61b6b84
Compare
|
@cevian @JLockerman updated the PR with the latest comments and created a new issue for Joshes comment: #128 |
| @@ -168,3 +169,29 @@ func (l *Labels) Swap(i, j int) { | |||
| l.values[j] = l.values[i] | |||
| l.values[i] = tmp | |||
| } | |||
|
|
|||
| // FromLabelMatchers parses protobuf label matchers to Prometheus label matchers. | |||
| func FromLabelMatchers(matchers []*prompb.LabelMatcher) ([]*labels.Matcher, error) { | |||
There was a problem hiding this comment.
Doesn't really belong here either, this is for our own Labels datatype 🤔
By using the Prometheus QueryEngine we enable
Timescale-Prometheus to return results for query and query_range
PromQL requests.