UI Support for Store filtering #3166

aribalam · 2020-09-14T17:11:17Z

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

Changes

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 · 2020-09-15T14:51:24Z

@bwplotka @prmsrswt Could you review this?

onprem · 2020-09-15T15:45:39Z

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!

onprem

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

pkg/ui/react-app/src/pages/graph/PanelList.tsx

pkg/ui/react-app/src/pages/graph/Panel.tsx

pkg/ui/react-app/src/pages/graph/PanelList.tsx

pkg/ui/react-app/src/pages/graph/Panel.tsx

onprem · 2020-09-15T18:22:20Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

@@ -91,6 +96,7 @@ class Panel extends Component<PanelProps & PathPrefixProps, PanelState> {
      maxSourceResolution,
      useDeduplication,
      usePartialResponse,
+      // TODO: Add support for Store Matches


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

aribalam · 2020-09-15T19:57:34Z

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

aribalam · 2020-09-16T19:41:16Z

@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 · 2020-09-16T20:04:53Z

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 · 2020-09-17T19:51:37Z

@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 · 2020-09-17T19:55:28Z

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

aribalam · 2020-09-18T15:27:57Z

@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 · 2020-09-18T18:19:35Z

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 · 2020-09-21T18:27:29Z

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

onprem

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.

onprem · 2020-09-21T18:49:36Z

pkg/ui/react-app/src/utils/utils.test.ts

        },
      },
    ];
    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';


I think we should only store the name of the store in the URI as the name is sufficient to identify a store uniquely.

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.

onprem · 2020-09-21T19:06:49Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

@@ -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 });


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).

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

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

bwplotka

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 · 2020-09-23T17:17:09Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

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

+    // add storeMatches to query params


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

bwplotka · 2020-09-23T17:17:56Z

pkg/ui/react-app/src/pages/graph/PanelList.tsx


  useEffect(() => {
+    // convert stores data to a unified stores array


bwplotka · 2020-09-23T17:20:35Z

Thanks for helping BTW ❤️

aribalam · 2020-09-23T19:51:28Z

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 · 2020-09-24T17:06:01Z

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 · 2020-09-24T17:37:58Z

This is perfect! Thanks 👍

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

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

onprem

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?

onprem · 2020-09-24T18:50:40Z

pkg/ui/react-app/src/pages/graph/Panel.tsx

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


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>

aribalam · 2020-09-24T19:52:54Z

@prmsrswt All fixed. :)

onprem · 2020-09-24T20:24:34Z

Great work! Thanks 🎉

bwplotka · 2020-09-28T14:09:56Z

🎉

onprem reviewed Sep 15, 2020

View reviewed changes

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

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

onprem approved these changes Sep 21, 2020

View reviewed changes

onprem reviewed Sep 21, 2020

View reviewed changes

bwplotka reviewed Sep 23, 2020

View reviewed changes

fetched lsit of stores in PanelList and passed into its content

c435466

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

aribalam added 19 commits September 25, 2020 00:03

added key prop for checkboxes

3658e84

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

added react-select to yarn

1d4c9c3

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

added react-select to package.json

8cbdd85

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

preprocess stores response data before passing as props to panels

20a4b5d

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

added dropdown ui for store selection

08121ef

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

removed redundant code

0d89425

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

added style for label and wrapper

df62297

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

added support to encode and decode storeMatch from URI params

471db93

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

removed linting errors

e4e9894

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

removed linting errors

063eff8

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

select component takes initial values

9fa8532

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

updated bindata.go

5853968

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

added store props and options in panels

62a5931

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

removed lint errors

126bac5

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

added PR to changelog

f5125c8

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

handle null case for selected stores

bbf2969

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

added checkbox for debug mode

1d12439

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

clear selected store on debug mode off

9e92d7e

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

changed checkbox name and corrected comment styles

ba86337

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

aribalam force-pushed the store_match_ui branch from 8eae99b to ba86337 Compare September 24, 2020 18:37

onprem reviewed Sep 24, 2020

View reviewed changes

removed ternary checking

06bfef5

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

aribalam force-pushed the store_match_ui branch from 8a74f4d to 2ed2f47 Compare September 24, 2020 19:24

aribalam added 2 commits September 25, 2020 00:59

removed lint errors

3adef70

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

make assets

02534b9

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

aribalam force-pushed the store_match_ui branch from 2ed2f47 to 02534b9 Compare September 24, 2020 19:31

onprem merged commit 9d638e9 into thanos-io:master Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI Support for Store filtering #3166

UI Support for Store filtering #3166

aribalam commented Sep 14, 2020 •

edited

Loading

aribalam commented Sep 15, 2020

onprem commented Sep 15, 2020

onprem left a comment •

edited

Loading

onprem Sep 15, 2020

aribalam commented Sep 15, 2020

aribalam commented Sep 16, 2020

onprem commented Sep 16, 2020

aribalam commented Sep 17, 2020

onprem commented Sep 17, 2020

aribalam commented Sep 18, 2020

onprem commented Sep 18, 2020

aribalam commented Sep 21, 2020

onprem left a comment •

edited

Loading

onprem Sep 21, 2020

aribalam Sep 21, 2020

onprem Sep 21, 2020

aribalam Sep 21, 2020

onprem Sep 22, 2020

bwplotka left a comment

bwplotka Sep 23, 2020

bwplotka Sep 23, 2020

bwplotka commented Sep 23, 2020

aribalam commented Sep 23, 2020

onprem commented Sep 24, 2020

bwplotka commented Sep 24, 2020

onprem left a comment •

edited

Loading

onprem Sep 24, 2020

aribalam commented Sep 24, 2020

onprem commented Sep 24, 2020

bwplotka commented Sep 28, 2020


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

UI Support for Store filtering #3166

UI Support for Store filtering #3166

Conversation

aribalam commented Sep 14, 2020 • edited Loading

Changes

Verification

aribalam commented Sep 15, 2020

onprem commented Sep 15, 2020

onprem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aribalam commented Sep 15, 2020

aribalam commented Sep 16, 2020

onprem commented Sep 16, 2020

aribalam commented Sep 17, 2020

onprem commented Sep 17, 2020

aribalam commented Sep 18, 2020

onprem commented Sep 18, 2020

aribalam commented Sep 21, 2020

onprem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Sep 23, 2020

aribalam commented Sep 23, 2020

onprem commented Sep 24, 2020

bwplotka commented Sep 24, 2020

onprem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aribalam commented Sep 24, 2020

onprem commented Sep 24, 2020

bwplotka commented Sep 28, 2020

aribalam commented Sep 14, 2020 •

edited

Loading

onprem left a comment •

edited

Loading

onprem left a comment •

edited

Loading

onprem left a comment •

edited

Loading