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

Support downsampling for /series. #2003

Merged
merged 1 commit into from
Feb 6, 2020

Conversation

IKSIN
Copy link
Contributor

@IKSIN IKSIN commented Jan 15, 2020

Signed-off-by: Aleksey Sin [email protected]

Use-case

Grafana need to get variables from /series. It do not work when we have only downsampled data (we using other retentions for every resolutions).

Changes

Support downsampling for /series API method. Based on defaultInstantQueryMaxSourceResolution

Verification

Grafana can get data for variables.

@IKSIN IKSIN force-pushed the series_downsampling branch from 70ef277 to 7b59bca Compare January 15, 2020 10:23
@IKSIN IKSIN requested a review from povilasv January 15, 2020 11:39
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

LGTM but could we also add a changelog entry + update the HELP string in cmd/thanos/query.go somehow so that our users would know that this applies for api/v1/series requests as well?

@IKSIN IKSIN force-pushed the series_downsampling branch from 17e03c0 to f2062ae Compare January 17, 2020 08:51
@IKSIN IKSIN requested a review from GiedriusS January 17, 2020 08:52
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

I think I would be OK with this code and reusing the same parameter but let's wait a bit to hear what others think.

@GiedriusS GiedriusS requested a review from bwplotka January 22, 2020 19:06
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.

This is kind of weird as series does not fetch chunks... I think the fix here is to actually look up all blocks even downsampled for all series? Instead of this?

What if instead put maxSourceResolution = max.Int64`? I think that should include all series... (:
What do you think?

@GiedriusS
Copy link
Member

This is kind of weird as series does not fetch chunks... I think the fix here is to actually look up all blocks even downsampled for all series? Instead of this?

What if instead put maxSourceResolution = max.Int64`? I think that should include all series... (:
What do you think?

Yep, we could just do this here. It would reduce the confusion for the users. 👍

@IKSIN IKSIN force-pushed the series_downsampling branch from f2062ae to 5597322 Compare February 3, 2020 09:37
@IKSIN IKSIN requested review from bwplotka and GiedriusS February 3, 2020 10:23
@IKSIN
Copy link
Contributor Author

IKSIN commented Feb 3, 2020

This is kind of weird as series does not fetch chunks... I think the fix here is to actually look up all blocks even downsampled for all series? Instead of this?

What if instead put maxSourceResolution = max.Int64`? I think that should include all series... (:
What do you think?

Yes, I think it good idea! PR Updated)

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Should be good since we do not retrieve the chunks themselves 👍

@IKSIN
Copy link
Contributor Author

IKSIN commented Feb 6, 2020

@bwplotka Any comments?)

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.

No. Did you check if this works? ;p @IKSIN

It looks good to me. Simple and powerful 👍

@GiedriusS
Copy link
Member

I have tested this out - works properly and now returns all data. Thanks!

@GiedriusS GiedriusS merged commit cf558de into thanos-io:master Feb 6, 2020
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