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

UI Support for Store filtering #3166

Merged
merged 37 commits into from
Sep 24, 2020
Merged

Conversation

aribalam
Copy link
Contributor

@aribalam aribalam commented Sep 14, 2020

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

Changes

Fixes #3096

Added support for Store filtering / Matching through the new React UI Interface.

The PR makes changes in the two files PanelList.tsx and Panel.tsx. The PanelList component retrieves a list of stores from the API and passes it to the Panels as props.

The Panels creates checkboxes for each store that determines whether the store should be selected for querying. The query, on execution, creates appropriate URL Params for store selection (using the storeMatch[] parameter) and displays the result.

Verification

Below is the screenshots of the new feature.


The query result when there is no store filtering



The query result when only the first store is selected


The query result multiple stores are selected


The query result when the third store is selected

@aribalam
Copy link
Contributor Author

@bwplotka @prmsrswt Could you review this?

@onprem
Copy link
Member

onprem commented Sep 15, 2020

Awesome work! I haven't had the time to review it fully yet, just a few nits the came to my mind

  • The checkboxes are fine for a small number of stores but it can take a lot more space if there are large number of stores (which is not that uncommon). A better UX would be to use something like a multiselect.

  • The tests are throwing linter errors, you can install the eslint extension in your IDE so that it can warn you when you are writing the code. It'd trivial to fix them though, just some spacing and indentation errors.

  • Thanos ships the generated react build as static asset in the binary itself. For this we need to run make assets, ideally at the end of the PR, so whenever we make changes we need to run make assets which would generate the assets required. I think you missed this step so just run make assets in the project root and commit the changes.

Thanks for the PR!

Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

Overall amazing work! There are just a few nits from my side.

Also, it would be nice if you can add a few tests here and there so that we don't break this in the future.

One more thing; we can also add the storeMatches parameter to the URI on the client-side as well. This way it'd be also persisted if we refresh the page etc. @squat @GiedriusS WDYT? @aribalam If you want to dabble into this, take a look at parseOption and toQueryString functions in src/uitls/index.ts.

pkg/ui/react-app/src/pages/graph/Panel.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/pages/graph/Panel.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/pages/graph/PanelList.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/pages/graph/Panel.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/pages/graph/PanelList.tsx Outdated Show resolved Hide resolved
pkg/ui/react-app/src/pages/graph/Panel.tsx Outdated Show resolved Hide resolved
@@ -91,6 +96,7 @@ class Panel extends Component<PanelProps & PathPrefixProps, PanelState> {
maxSourceResolution,
useDeduplication,
usePartialResponse,
// TODO: Add support for Store Matches
Copy link
Member

Choose a reason for hiding this comment

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

It'd nice to see this get done in this PR, but it's not a blocker :)

@aribalam
Copy link
Contributor Author

@prmsrswt Thanks for the review! will change all the mentioned points as soon as possible! :)

@aribalam
Copy link
Contributor Author

@prmsrswt Using multi-select is the best way to go. However, reactstrap does not have any similar implementation of it. We could use any third party library for this, but the look-and-feel would be different from the general theme of the application. We could try to implement on our own using dropdowns, but that would be pretty much complex.
Any thought on how could I proceed?

@onprem
Copy link
Member

onprem commented Sep 16, 2020

Writing our own implementation for multiselect would be a waste of effort IMO. There are some pretty good libraries out there, just pick one that fits best for our use case and has a reasonably low footprint.
Many libraries allow us to customise the appearance using CSS, so we can try to make it fit better, but it's not a major concern.

@aribalam
Copy link
Contributor Author

@prmsrswt This is the UI that I have proposed to add. Please have a look.

I am using a third-party library named React Select to achieve this (link here - https://react-select.com/home)
I will proceed to commit this if you give a green signal.

@onprem
Copy link
Member

onprem commented Sep 17, 2020

This looks great, please go ahead. Thanks for your work.

@aribalam aribalam force-pushed the store_match_ui branch 7 times, most recently from c252451 to 0f18fcf Compare September 18, 2020 14:33
@aribalam
Copy link
Contributor Author

@prmsrswt I have successfully integrated the store filter using the new UI.

One more thing; we can also add the storeMatches parameter to the URI on the client-side as well. This way it'd be also persisted if we refresh the page etc. @squat @GiedriusS WDYT? @aribalam If you want to dabble into this, take a look at parseOption and toQueryString functions in src/uitls/index.ts.

I have also taken care of the above. However, I am not able to build the latest version of this using make assets. Apparently, it is not able to detect new changes. Also, some of the React UI test cases seem to fail which I am not able to fix it.

Could you take a look at this?

@onprem
Copy link
Member

onprem commented Sep 18, 2020

You can try to build manually, try running scripts/build-react-app.sh and it'll build and copy the files in the proper place for you, then just run make assets.

The React tests are failing because you haven't updated the tests to expect the new query parameter you added (storeMatches). If you face any more problems, just ping me on slack.

@aribalam aribalam force-pushed the store_match_ui branch 4 times, most recently from a1d1a22 to 699171e Compare September 21, 2020 18:03
@aribalam
Copy link
Contributor Author

@prmsrswt The test cases are passing finally!. :)
I have also updated the changelog. Please let me know if you need any more changes.

Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

Good work! Just a small nit, not a blocker though, can be done in a separate PR.

Edit: Found a bug that was crashing things, should be simple enough to fix.

},
},
];
const query =
'?g0.expr=rate(node_cpu_seconds_total%7Bmode%3D%22system%22%7D%5B1m%5D)&g0.tab=0&g0.stacked=0&g0.range_input=1h&g0.max_source_resolution=raw&g0.deduplicate=1&g0.partial_response=0&g0.end_input=2019-10-25%2023%3A37%3A00&g0.moment_input=2019-10-25%2023%3A37%3A00&g1.expr=node_filesystem_avail_bytes&g1.tab=1&g1.stacked=0&g1.range_input=1h&g1.max_source_resolution=auto&g1.deduplicate=0&g1.partial_response=1';
'?g0.expr=rate(node_cpu_seconds_total%7Bmode%3D%22system%22%7D%5B1m%5D)&g0.tab=0&g0.stacked=0&g0.range_input=1h&g0.max_source_resolution=raw&g0.deduplicate=1&g0.partial_response=0&g0.store_matches=%5B%5D&g0.end_input=2019-10-25%2023%3A37%3A00&g0.moment_input=2019-10-25%2023%3A37%3A00&g1.expr=node_filesystem_avail_bytes&g1.tab=1&g1.stacked=0&g1.range_input=1h&g1.max_source_resolution=auto&g1.deduplicate=0&g1.partial_response=1&g1.store_matches=%5B%7B%22name%22%3A%22thanos_sidecar_one%3A10901%22%2C%22lastCheck%22%3A%222020-09-20T11%3A35%3A18.250713478Z%22%2C%22lastError%22%3Anull%2C%22labelSets%22%3A%5B%7B%22labels%22%3A%5B%7B%22name%22%3A%22monitor%22%2C%22value%22%3A%22prometheus_one%22%7D%5D%7D%5D%2C%22minTime%22%3A1600598100000%2C%22maxTime%22%3A9223372036854776000%7D%5D';
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 only store the name of the store in the URI as the name is sufficient to identify a store uniquely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I too thought of passing only the store name, but the Select component that is being used takes in only objects as options. So passing a string array was not an option. I can create an interface with only store name as the key though. Would like your opinion on this.

@@ -255,8 +268,12 @@ class Panel extends Component<PanelProps & PathPrefixProps, PanelState> {
this.setOptions({ usePartialResponse: event.target.checked });
};

handleStoreMatchChange = (selectedStores: any): void => {
this.setOptions({ storeMatches: selectedStores });
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I found a non-obvious bug here. If you add some stores to the filter and then remove all of them selectedStores becomes null instead of [].

Modify this line to this.setOptions({ storeMatches: selectedStores || [] });

Also, just to be on the safer side, change line 152 to expect storeMatchers as null. Can be done easily using optional chaining; this.props.options.storeMatches?.forEach((store: Store) => (notice the ? after storeMatches).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! That one line check using ? was really cool! :)

Copy link
Member

Choose a reason for hiding this comment

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

You forget to run make assets though 😅 so the production build is still buggy.

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 for this. We definitely need something like this, just before we merge, wonder if we should make it less visible 🤔

Is there a quick way we can hide this option totally under some debug button? It takes quite space and should be used very very rarely -> only for rare debug use cases.

@@ -138,6 +146,11 @@ class Panel extends Component<PanelProps & PathPrefixProps, PanelState> {
partial_response: this.props.options.usePartialResponse.toString(),
});

// add storeMatches to query params
Copy link
Member

Choose a reason for hiding this comment

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

Comments should be full sentence? (: (capital letter, trailing period)


useEffect(() => {
// convert stores data to a unified stores array
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@bwplotka
Copy link
Member

Thanks for helping BTW ❤️

@aribalam
Copy link
Contributor Author

Thanks for this. We definitely need something like this, just before we merge, wonder if we should make it less visible

Is there a quick way we can hide this option totally under some debug button? It takes quite space and should be used very very rarely -> only for rare debug use cases.

@bwplotka I have added a debug checkbox beside the Enable query history and Use local time button at the top of the panel list. The store filter UI is visible only when it is checked. I have attached a few snaps below. Please have a look. I will push the changes if you approve it.


The UI when the debug mode is not checked.


The UI when the debug mode is checked.

@onprem
Copy link
Member

onprem commented Sep 24, 2020

I guess this checkbox will do. Maybe name the checkbox Enable store filtering instead of Debug mode so that people can understand what it does.

@bwplotka
Copy link
Member

This is perfect! Thanks 👍

Let's rebase and add @prmsrswt suggestion and LGTM ❤️

Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

I guess the bindata.go wasn't able to survive a rebase XD. I have a small nit, just implement it and run make assets again, the CI should pass after this. Oh there are some lint errors too, can you also fix them?

@@ -296,6 +313,23 @@ class Panel extends Component<PanelProps & PathPrefixProps, PanelState> {
</Checkbox>
</Col>
</Row>
{stores.length > 0 ? (<Row>
Copy link
Member

Choose a reason for hiding this comment

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

Cases like this are called conditional rendering in React and it's better to use && instead of the ternary operator.

So instead of {condition ? <Component /> : ""} we can just do {condition && <Component />}

Also, if stores is null or undefined here, it'll cause and error too, you can do {stores?.length > 0 .... to prevent this.

Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
Signed-off-by: aribalam <arib.alam.iitkgp@gmail.com>
@aribalam
Copy link
Contributor Author

@prmsrswt All fixed. :)

@onprem
Copy link
Member

onprem commented Sep 24, 2020

Great work! Thanks 🎉

@onprem onprem merged commit 9d638e9 into thanos-io:master Sep 24, 2020
@bwplotka
Copy link
Member

🎉

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.

query: Expose Store filtering in UI
3 participants