Generalise some of Mimir's query sharding code to be more reusable #9363
Conversation
zenador
force-pushed
the
zenador/prep-ccqf-middleware
branch
from
790131c
to
242223e
Compare
zenador
force-pushed
the
zenador/prep-ccqf-middleware
branch
from
242223e
to
078f01e
Compare
zenador
force-pushed
the
zenador/prep-ccqf-middleware
branch
from
078f01e
to
6f1e4ed
Compare
| @@ -755,6 +817,90 @@ func (c prometheusCodec) DecodeResponse(ctx context.Context, r *http.Response, _ | |||
| return resp, nil | |||
| } | |||
|
|
|||
| func (c prometheusCodec) DecodeLabelsQueryResponse(ctx context.Context, r *http.Response, lr LabelsQueryRequest, logger log.Logger) (Response, error) { | |||
There was a problem hiding this comment.
Should this be split in half, one half for labels and another for series responses?
Or is there a reason to keep them in one method like this?
(same applies for EncodeLabelsQueryResponse)
There was a problem hiding this comment.
I kept it as one because the LabelsQueryRequest is used for label names, label values and series because their request formats are all fairly similar and this function outputs a Response which works for all three hence the signatures are the same, and I don't want to clutter this with more functions that do almost the same thing, and only differentiate between the labels and series in the formatter interface (they have different signatures since the response formats are different).
There was a problem hiding this comment.
fwiw I tend to agree with Charles. It's less surprising if we have the separation in all places. Now DecodeLabelsResponse and DecodeLabelsQueryResponse work for different sets of requests.
| name: "successful series response", | ||
| request: &PrometheusSeriesQueryRequest{}, |
There was a problem hiding this comment.
should this also test for label names?
| @@ -755,6 +817,90 @@ func (c prometheusCodec) DecodeResponse(ctx context.Context, r *http.Response, _ | |||
| return resp, nil | |||
| } | |||
|
|
|||
| func (c prometheusCodec) DecodeLabelsQueryResponse(ctx context.Context, r *http.Response, lr LabelsQueryRequest, logger log.Logger) (Response, error) { | |||
There was a problem hiding this comment.
fwiw I tend to agree with Charles. It's less surprising if we have the separation in all places. Now DecodeLabelsResponse and DecodeLabelsQueryResponse work for different sets of requests.
| isSeriesResponse: false, | ||
| }, | ||
| { | ||
| name: "successful series response", |
There was a problem hiding this comment.
same as above - test with label names too?
| Path string | ||
| Start int64 | ||
| End int64 | ||
| // labelMatcherSets is a repeated field here in order to enable the representation |
There was a problem hiding this comment.
nit: labelMatcherSets->LabelMatcherSets to make it a godoc
| func (r *PrometheusSeriesQueryRequest) GetStartOrDefault() int64 { | ||
| if r.GetStart() == 0 { | ||
| return v1.MinTime.UnixMilli() | ||
| } | ||
| return r.GetStart() | ||
| } | ||
|
|
||
| func (r *PrometheusSeriesQueryRequest) GetEndOrDefault() int64 { | ||
| if r.GetEnd() == 0 { | ||
| return v1.MaxTime.UnixMilli() | ||
| } | ||
| return r.GetEnd() | ||
| } | ||
|
|
||
| func (r *PrometheusLabelNamesQueryRequest) GetHeaders() []*PrometheusHeader { | ||
| return r.Headers | ||
| } | ||
|
|
||
| func (r *PrometheusLabelValuesQueryRequest) GetHeaders() []*PrometheusHeader { | ||
| return r.Headers | ||
| } | ||
|
|
||
| func (r *PrometheusSeriesQueryRequest) GetHeaders() []*PrometheusHeader { | ||
| return r.Headers | ||
| } | ||
|
|
||
| // WithLabelName clones the current `PrometheusLabelNamesQueryRequest` with a new label name param. | ||
| func (r *PrometheusLabelNamesQueryRequest) WithLabelName(string) (LabelsQueryRequest, error) { | ||
| panic("PrometheusLabelNamesQueryRequest.WithLabelName is not implemented") | ||
| } | ||
|
|
||
| // WithLabelName clones the current `PrometheusLabelValuesQueryRequest` with a new label name param. | ||
| func (r *PrometheusLabelValuesQueryRequest) WithLabelName(name string) (LabelsQueryRequest, error) { | ||
| newRequest := *r | ||
| newRequest.Path = labelValuesPathSuffix.ReplaceAllString(r.Path, `/api/v1/label/`+name+`/values`) | ||
| newRequest.LabelName = name | ||
| return &newRequest, nil | ||
| } | ||
|
|
||
| // WithLabelName clones the current `PrometheusSeriesQueryRequest` with a new label name param. | ||
| func (r *PrometheusSeriesQueryRequest) WithLabelName(string) (LabelsQueryRequest, error) { | ||
| panic("PrometheusSeriesQueryRequest.WithLabelName is not implemented") | ||
| } | ||
|
|
||
| // WithLabelMatcherSets clones the current `PrometheusLabelNamesQueryRequest` with new label matcher sets. | ||
| func (r *PrometheusLabelNamesQueryRequest) WithLabelMatcherSets(labelMatcherSets []string) (LabelsQueryRequest, error) { | ||
| newRequest := *r | ||
| newRequest.LabelMatcherSets = make([]string, len(labelMatcherSets)) | ||
| copy(newRequest.LabelMatcherSets, labelMatcherSets) | ||
| return &newRequest, nil | ||
| } | ||
|
|
||
| // WithLabelMatcherSets clones the current `PrometheusLabelValuesQueryRequest` with new label matcher sets. | ||
| func (r *PrometheusLabelValuesQueryRequest) WithLabelMatcherSets(labelMatcherSets []string) (LabelsQueryRequest, error) { | ||
| newRequest := *r | ||
| newRequest.LabelMatcherSets = make([]string, len(labelMatcherSets)) | ||
| copy(newRequest.LabelMatcherSets, labelMatcherSets) | ||
| return &newRequest, nil | ||
| } | ||
|
|
||
| // WithLabelMatcherSets clones the current `PrometheusSeriesQueryRequest` with new label matcher sets. | ||
| func (r *PrometheusSeriesQueryRequest) WithLabelMatcherSets(labelMatcherSets []string) (LabelsQueryRequest, error) { | ||
| newRequest := *r | ||
| newRequest.LabelMatcherSets = make([]string, len(labelMatcherSets)) | ||
| copy(newRequest.LabelMatcherSets, labelMatcherSets) | ||
| return &newRequest, nil | ||
| } | ||
|
|
||
| // WithHeaders clones the current `PrometheusLabelNamesQueryRequest` with new headers. | ||
| func (r *PrometheusLabelNamesQueryRequest) WithHeaders(headers []*PrometheusHeader) (LabelsQueryRequest, error) { | ||
| newRequest := *r | ||
| newRequest.Headers = cloneHeaders(headers) | ||
| return &newRequest, nil | ||
| } | ||
|
|
||
| // WithHeaders clones the current `PrometheusLabelValuesQueryRequest` with new headers. | ||
| func (r *PrometheusLabelValuesQueryRequest) WithHeaders(headers []*PrometheusHeader) (LabelsQueryRequest, error) { | ||
| newRequest := *r | ||
| newRequest.Headers = cloneHeaders(headers) | ||
| return &newRequest, nil | ||
| } | ||
|
|
||
| // WithHeaders clones the current `PrometheusSeriesQueryRequest` with new headers. | ||
| func (r *PrometheusSeriesQueryRequest) WithHeaders(headers []*PrometheusHeader) (LabelsQueryRequest, error) { | ||
| newRequest := *r | ||
| newRequest.Headers = cloneHeaders(headers) | ||
| return &newRequest, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
nit on ordering: we usually put the methods of a struct below the struct not with methods of the same name of other structs. I see you did this for some of the methods, but not these ones here
What this PR does
Generalise some of Mimir's query sharding code to be more reusable for other kinds of sharded queries.
Which issue(s) this PR fixes or relates to
Follow up to #8755
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.No docs as these are minor, non-user-facing changes.