*: update Cortex (and Prometheus) dependency #4586
Conversation
Update Cortex dependency (and Prometheus together) to include cortexproject/cortex@70dddb6. storage.Querier finally has support for label.Matchers so now we can fully pass them down. Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS
force-pushed
the
update_cortex_prometheus
branch
from
6d283c0
to
614e1b1
Compare
|
Nice! Is this pr ready for review now? |
No, still need to fix the tests hence it is a draft |
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
GiedriusS
force-pushed
the
update_cortex_prometheus
branch
from
600c169
to
8f9a57f
Compare
| } | ||
| sort.Strings(res) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is probably the biggest change - in this function 😄 We have to add external labels here now so that the behaviour would be the same as previously. We were doing Select() and then picking label names out of all series so naturally, they would have external labels there.
|
@yeya24 fixed the tests, PTAL 🍻 |
pkg/api/query/v1.go
Outdated
| @@ -657,13 +657,13 @@ func (qapi *QueryAPI) labelNames(r *http.Request) (interface{}, []error, *api.Ap | |||
| return nil, nil, apiErr | |||
| } | |||
|
|
|||
| var matcherSets [][]*labels.Matcher | |||
| var matcherSets []*labels.Matcher | |||
There was a problem hiding this comment.
The behavior changes here. We need to support multiple matchers, same as the label values API.
Signed-off-by: Giedrius Statkevičius <[email protected]>
|
@yeya24 thanks for the very quick review! ❤️ |
Signed-off-by: Giedrius Statkevičius <[email protected]>
| @@ -702,7 +702,7 @@ func (c *Client) SeriesInGRPC(ctx context.Context, base *url.URL, matchers []*la | |||
| return m.Data, c.get2xxResultWithGRPCErrors(ctx, "/prom_series HTTP[client]", &u, &m) | |||
| } | |||
|
|
|||
| // LabelNames returns all known label names. It uses gRPC errors. | |||
| // LabelNames returns all known label names constrained by the given matchers. It uses gRPC errors. | |||
| // NOTE: This method is tested in pkg/store/prometheus_test.go against Prometheus. | |||
| func (c *Client) LabelNamesInGRPC(ctx context.Context, base *url.URL, matchers []storepb.LabelMatcher, startTime, endTime int64) ([]string, error) { | |||
There was a problem hiding this comment.
This is still not sufficient I think. If the Prometheus server doesn't support label name matchers, then all label names will be returned. What about doing the same as https://github.com/thanos-io/thanos/blob/main/pkg/store/prometheus.go#L533-L567? Checking Prometheus version seems safer here.
@GiedriusS @bwplotka
There was a problem hiding this comment.
Yea, some compatibility code has to be there, thanks for being careful. I am on PTO until Monday, will catch on this unless someone else does 🤗
* *: update Cortex (and Prometheus) dependency Update Cortex dependency (and Prometheus together) to include cortexproject/cortex@70dddb6. storage.Querier finally has support for label.Matchers so now we can fully pass them down. Signed-off-by: Giedrius Statkevičius <[email protected]> * *: fix tests by passing EnableExemplarStorage Signed-off-by: Giedrius Statkevičius <[email protected]> * CHANGELOG: update with note about LabelNames() pushdown Signed-off-by: Giedrius Statkevičius <[email protected]> * store: pass along matchers in proxy Signed-off-by: Giedrius Statkevičius <[email protected]> * store: fix e2e labelname tests Signed-off-by: Giedrius Statkevičius <[email protected]> * Fix according to Ben's comments Signed-off-by: Giedrius Statkevičius <[email protected]> * test/e2e: fix grammar issue in a comment Signed-off-by: Giedrius Statkevičius <[email protected]>
Update Cortex dependency (and Prometheus together) to include
cortexproject/cortex@70dddb6.
storage.Querier finally has support for label.Matchers so now we can
fully pass them down. This is covered by tests. Plus, I've played around a bit
with these changes locally.
Signed-off-by: Giedrius Statkevičius [email protected]