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

Add time range parameters to labels API #2964

Merged
merged 2 commits into from
Aug 6, 2020

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Jul 31, 2020

Signed-off-by: Ben Ye yb532204897@gmail.com

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

Changes

Fixes: #1811

  • Add changelog
  • Update unit tests and E2E tests.

Verification

@yeya24 yeya24 force-pushed the add-time-params-label-api branch from f5773fe to bc9005d Compare July 31, 2020 15:41
@yeya24 yeya24 changed the title WIP: add time range parameters to labels API Add time range parameters to labels API Jul 31, 2020
@yeya24
Copy link
Contributor Author

yeya24 commented Jul 31, 2020

This pr is ready for review!

@yeya24 yeya24 force-pushed the add-time-params-label-api branch from bc9005d to c6661b4 Compare July 31, 2020 15:47
@yeya24 yeya24 requested review from pracucci, bwplotka and jojohappy and removed request for pracucci July 31, 2020 15:47
@yeya24 yeya24 force-pushed the add-time-params-label-api branch 6 times, most recently from 0da6da5 to 91d8a5c Compare August 1, 2020 02:45
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.

Amazing, good work! 💪 It's perfect really just one comment (:

if err != nil {
return nil, nil, &api.ApiError{Typ: api.ErrorBadData, Err: err}
}
// Prometheus doesn't check this, do we need to?
Copy link
Member

Choose a reason for hiding this comment

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

Sounds useful, no? (:

Please ask comments on PR itself not in comment (: So we can still merge PR if code is ok.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should and it's probably worth contributing such a check upstream IMHO

@@ -144,6 +144,10 @@ message LabelNamesRequest {

// TODO(bwplotka): Move Thanos components to use strategy instead. Including QueryAPI.
PartialResponseStrategy partial_response_strategy = 2;

int64 start = 3;
Copy link
Member

Choose a reason for hiding this comment

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

🤙

Signed-off-by: Ben Ye <yb532204897@gmail.com>
@yeya24
Copy link
Contributor Author

yeya24 commented Aug 2, 2020

I have addressed the comment. Please take a look again. 😄

Signed-off-by: Ben Ye <yb532204897@gmail.com>
Copy link
Member

@jojohappy jojohappy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@yeya24 yeya24 requested a review from bwplotka August 6, 2020 12:49
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.

Thanks!

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.

querier: Limit LabelNames; LabelValues to certain time period.
4 participants