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

Update prometheus and cortex #3804

Merged
merged 7 commits into from
Feb 19, 2021

Conversation

replay
Copy link
Contributor

@replay replay commented Feb 15, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This updates the vendored Prometheus and Cortex versions. In the latest version of Prometheus the Querier's .LabelValues() call now takes a matchers parameter, so this also adds that parameter to the storepb.LabelValuesRequest type.

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay replay force-pushed the update_prometheus_and_cortex branch from 44057f0 to 7a5126c Compare February 15, 2021 19:28
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay replay force-pushed the update_prometheus_and_cortex branch 3 times, most recently from a5c056e to 1f1f54d Compare February 16, 2021 12:08
@replay replay changed the title [WIP] Update prometheus and cortex Update prometheus and cortex Feb 16, 2021
@replay replay marked this pull request as ready for review February 16, 2021 12:23
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay replay force-pushed the update_prometheus_and_cortex branch from 1f1f54d to 6c3a35e Compare February 16, 2021 12:24
span, ctx := tracing.StartSpan(q.ctx, "querier_label_values")
defer span.Finish()

// TODO(bwplotka): Pass it using the SeriesRequest instead of relying on context.
ctx = context.WithValue(ctx, store.StoreMatcherKey, q.storeDebugMatchers)

pbMatchers, err := storepb.PromMatchersToMatchers(matchers...)
if err != nil {
return nil, nil, errors.Wrap(err, "proxy LabelValues()")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update the msg to convert matchers to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

@@ -178,6 +178,8 @@ message LabelValuesRequest {
// The content of this field and whether it's supported depends on the
// implementation of a specific store.
google.protobuf.Any hints = 6;

repeated LabelMatcher label_matchers = 7 [(gogoproto.nullable) = false];
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just call it matchers to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thank you! Some consistency fixes to address and we can merge! 💪🏽

LGTM when fixed. Would you have time to address those comments? I can then release Thanos 0.19 with this 💪🏽

span, ctx := tracing.StartSpan(q.ctx, "querier_label_values")
defer span.Finish()

// TODO(bwplotka): Pass it using the SeriesRequest instead of relying on context.
ctx = context.WithValue(ctx, store.StoreMatcherKey, q.storeDebugMatchers)

pbMatchers, err := storepb.PromMatchersToMatchers(matchers...)
if err != nil {
return nil, nil, errors.Wrap(err, "proxy LabelValues()")
Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

@@ -178,6 +178,8 @@ message LabelValuesRequest {
// The content of this field and whether it's supported depends on the
// implementation of a specific store.
google.protobuf.Any hints = 6;

repeated LabelMatcher label_matchers = 7 [(gogoproto.nullable) = false];
Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

@replay
Copy link
Contributor Author

replay commented Feb 17, 2021

LGTM when fixed. Would you have time to address those comments? I can then release Thanos 0.19 with this

Great! yes I'll make the requested changes, thanks to both of you for the quick feedback.

@replay replay force-pushed the update_prometheus_and_cortex branch from db5407b to 7f9c323 Compare February 18, 2021 18:45
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
@replay replay force-pushed the update_prometheus_and_cortex branch from 7f9c323 to bd10173 Compare February 18, 2021 19:00
@replay
Copy link
Contributor Author

replay commented Feb 18, 2021

I have addressed the comments, please take another look

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@yeya24 yeya24 merged commit e80836f into thanos-io:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants