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.

Sign up for GitHub

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

Jump to bottom

Fix downsampling option in querier URL #1562

Merged
merged 3 commits into from Oct 28, 2019
Merged

Fix downsampling option in querier URL #1562

merged 3 commits into from Oct 28, 2019

Conversation

obiesmans
Copy link
Contributor

@obiesmans obiesmans commented Sep 24, 2019 •
edited

Fixes #1463

  • Downsampling option in Querier UI is now reflected in the URL.

Changes

Verification

@obiesmans
Copy link
Contributor Author

Amended the pr to use camelcase and have a max_source_resolution in URL, as in the documentation.

GiedriusS
GiedriusS reviewed Sep 28, 2019
View reviewed changes
pkg/ui/static/js/graph_template.handlebar Outdated Show resolved Hide resolved
pkg/ui/static/js/graph.js Outdated Show resolved Hide resolved
bwplotka
bwplotka reviewed Oct 2, 2019
View reviewed changes
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.

Awesome, thanks. Generally, it looks ok, just @GiedriusS comments applies. (:

Have you verified Querier and checked if it works? (:

@GiedriusS
Copy link
Member

Everything works, just those two comments and we should be good to go 😄

Fix downsampling option in querier URL
bd660a3 
Signed-off-by: Olivier Biesmans <olivier.biesmans@blablacar.com>
@obiesmans
Copy link
Contributor Author

Thanks a lot for the review ! Sorry I didn't reply sooner, was off for a few days.
I pushed an update that addresses @GiedriusS comments.

I also ran into issues where the URL params format didn't allow for empty params.
I thought that correcting this would make maintaining against the upstream prometheus UI harder, so instead I put "auto" as keyword in the UI and normalized it back to an empty string when submitting the query. Is it OK with you ?

bwplotka
bwplotka reviewed Oct 17, 2019
View reviewed changes
pkg/ui/static/js/graph.js Outdated Show resolved Hide resolved
Fix downsampling option in querier URL
b949fe1 
Signed-off-by: Olivier Biesmans <olivier.biesmans@blablacar.com>
bwplotka
bwplotka reviewed Oct 17, 2019
View reviewed changes
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.

Generally good, last question hopefully (:

pkg/ui/static/js/graph_template.handlebar
<option value="">Auto downsampling</option>
<option selected="selected" value="0s">Only raw data</option>
<option value="auto">Auto downsampling</option>
<option value="0s">Only raw data</option>
Copy link
Member

Choose a reason for hiding this comment

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

Quick silly question, why we remove selected here again? It changes what is by default set, right?

Copy link
Contributor Author

@obiesmans obiesmans Oct 21, 2019 •
edited

Choose a reason for hiding this comment

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

I think that which option is selected by default should be driven from the javascript only.
(same place where the other UI driving logic is).

It doesn't change, the default which is still "0s" if the parameter is not set :
https://github.com/thanos-io/thanos/pull/1562/files#diff-b1f64ac291243b832fd99d257dcb04feR47-R49

Let me know if you agree with this choice, or if I should add this back in the UI.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense IMHO that we keep all of that logic together in that file instead of the HTML code, thanks for pointing this out!

GiedriusS
GiedriusS approved these changes Oct 28, 2019
View reviewed changes
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.

Played around with it and it works well. Thanks for this and sorry for the late answer!

@GiedriusS
Copy link
Member

Also, double-checked the "instant query" side of this - everything should work well like before and now actually the user will be able to see that they are able to change this parameter -- it was "invisible" before.

@GiedriusS GiedriusS merged commit 340a3c1 into thanos-io:master Oct 28, 2019
@obiesmans
Copy link
Contributor Author

obiesmans commented Oct 30, 2019 •
edited

Thanks a lot for taking the time to review this @GiedriusS !

@bwplotka bwplotka mentioned this pull request Nov 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka left review comments

@GiedriusS GiedriusS GiedriusS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Downsampling option not contained in Thanos URL
3 participants
@obiesmans @GiedriusS @bwplotka