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

querier: Adds additional stats field in query api #4176

Merged
merged 6 commits into from
May 7, 2021

Conversation

someshkoli
Copy link
Contributor

@someshkoli someshkoli commented May 4, 2021

Signed-off-by: someshkoli [email protected]

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

Changes

Adds option to pass additional paramater stats in form data for query API.

Closes #4170

Verification

need to add some endpoint tests

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.

Thanks for working on this issue.
I think this can be enabled by default and we don't need a flag.

Also please add some unit tests.

@someshkoli
Copy link
Contributor Author

Thanks for working on this issue.
I think this can be enabled by default and we don't need a flag.

Also please add some unit tests.

Alright, ill update the changes

pkg/api/query/v1.go Outdated Show resolved Hide resolved
pkg/api/query/v1.go Outdated Show resolved Hide resolved
@someshkoli someshkoli changed the title Add additional query stat falg to query command Adds additional stats field in thanos query api May 6, 2021
@someshkoli
Copy link
Contributor Author

@yeya24 PTAL 🙌

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.

Some small nits. Also it would be good if you can update the changelog

pkg/api/query/v1.go Outdated Show resolved Hide resolved
pkg/api/query/v1.go Outdated Show resolved Hide resolved
pkg/api/query/v1_test.go Outdated Show resolved Hide resolved
@someshkoli
Copy link
Contributor Author

Also it would be good if you ca

Also a stupid question, when and when not to update changelog ?

@someshkoli someshkoli requested a review from yeya24 May 7, 2021 09:22
@kakkoyun kakkoyun changed the title Adds additional stats field in thanos query api querier: Adds additional stats field in query api May 7, 2021
@yeya24
Copy link
Contributor

yeya24 commented May 7, 2021

Hi @someshkoli, please rebase on main then we can get CI fixed.

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: someshkoli <[email protected]>
@yeya24 yeya24 enabled auto-merge (squash) May 7, 2021 16:51
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. Will merge on green.

@yeya24 yeya24 merged commit 2e79689 into thanos-io:main May 7, 2021
@someshkoli
Copy link
Contributor Author

LGTM. Will merge on green.

yey all greens 🎉

@yeya24
Copy link
Contributor

yeya24 commented May 7, 2021

Also it would be good if you ca

Also a stupid question, when and when not to update changelog ?

When your pr changes some known behaviors or are user-facing. IMO this change provides a new param so it is user-facing.

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.

Support optional query stats parameter
2 participants