ui: Add Stores page to React UI by onprem · Pull Request #2754 · thanos-io/thanos

onprem · 2020-06-12T08:07:56Z

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

Changes

Migrate the stores page to the React UI

Verification

Tested locally using both development build of react and the thanos binary with production build of react.

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

soniasingla

LGTM 🔥

GiedriusS · 2020-06-12T16:14:31Z

It crashes for me because in my API response I have {} for one lastError:

Error: Objects are not valid as a React child (found: object with keys {}). If you meant to render a collection of children, use an array instead.
    in span (created by Badge)
    in Badge (at StorePoolPanel.tsx:58)
    in td (at StorePoolPanel.tsx:58)
    in tr (at StorePoolPanel.tsx:47)
    in tbody (at StorePoolPanel.tsx:40)
    in table (created by Table)
    in Table (at StorePoolPanel.tsx:32)
    in div (created by Transition)
    in Transition (created by Collapse)
    in Collapse (at StorePoolPanel.tsx:31)
    in div (created by Container)
    in Container (at StorePoolPanel.tsx:27)
    in StorePoolPanel (at Stores.tsx:16)
    in StoreContent (at withStatusIndicator.tsx:45)
    in Unknown (at Stores.tsx:30)
    in Stores (at App.tsx:34)

There are some interesting errors, it seems. If I convert it into a string explicitly, I get:

fetching store info from XXX:13903: rpc error: code = DeadlineExceeded desc = latest balancer error: connection error: desc = "transport: Error while dialing dial tcp: lookup XXX on YYY:53: no such host"

So, I'm certain that it has some weird MarshalJSON() function that returns an empty map. My suggestion is to return strings from the API explicitly. But, I guess, we can improve the API in future PR. WDYT?

GiedriusS

Code LGTM, just one suggestion/reminder. Awesome work! I like how simple the implementation is.

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx

onprem · 2020-06-12T18:50:04Z

So, I'm certain that it has some weird MarshalJSON() function that returns an empty map. My suggestion is to return strings from the API explicitly. But, I guess, we can improve the API in future PR. WDYT?

Returning string explicitly is a good idea. And yeah sure, I'll keep a note of it and fix this in a future PR.

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

GiedriusS

Shouldn't we add a minimum time constant and use it? Because now we check for that huge value in the min time column. And we should add both checks to both columns because sometimes those values might appear in different columns when we don't want StoreAPIs to match anything e.g. when Thanos Store doesn't find any blocks or cannot do its synchronization

onprem · 2020-06-14T14:49:40Z

I am not sure what do we need to use as MIN_TIME constant. Maybe 0?

Also, please correct me if I am wrong. What I understand from this is that we should check if a particular field is greater than MAX_TIME and show infinity sign, else if it is less than the MIN_TIME, we should show minus infinity, otherwise just show the parsed date-time.

GiedriusS

I guess this should be good to go, just need the tests like talked on Slack :P Since this PR is small enough, we can put them together. I can't comment personally on the React code's nuances because I don't know much about it so I think @squat should look more into that part. Awesome work!

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

GiedriusS

Sorry for the late response! Played a bit with it, looks awesome. 🎉 ❤️ The tests are thorough and seem to cover all cases. It should be good to go, IMHO.

GiedriusS · 2020-06-25T13:36:28Z

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.test.tsx

+        const td = row.find({ 'data-testid': 'lastCheck' });
+        expect(td).toHaveLength(1);
+
+        if (parseTime(lastCheck) >= MAX_TIME) {


AFAICT the only condition that we don't check in the tests is this but we can add another case in a follow-up PR

squat

super good! only small comments but not blockers

squat · 2020-06-12T08:22:16Z

pkg/ui/react-app/src/thanos/Navbar.tsx

 const navConfig: { [component: string]: NavConfig[] } = {
-  query: [{ name: 'Graph', uri: '/new/graph' }],
+  query: [
+    { name: 'Graph', uri: '/new/graph' },


Can't wait till we can get rid of the"new" prefix :)

squat · 2020-06-12T08:24:15Z

pkg/ui/react-app/src/thanos/pages/stores/StoreLabels.tsx

+  return (
+    <ListGroup>
+      {labelSet.map(({ labels }, idx) => (
+        <ListGroupItem key={idx}>


One thing, for consistency let's try to use the same variable named for common things like indexes in loops. Not sure about the rest of the tsx codebase in the repo but let's just pick either i or idx

We haven't found any other loops that use some other name and other code uses idx as well

squat · 2020-06-25T13:53:35Z

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx

+                    <StoreLabels labelSets={labelSets} />
+                  </td>
+                  <td data-testid="minTime">
+                    {minTime >= MAX_TIME ? <FontAwesomeIcon icon={faInfinity} /> : formatTime(minTime)}


strictly speaking should this rather be > instead of >=?

Talked about this in our sync - this is OK for now. It would be worth investigating whether momentjs has some kind of constant or function that we could compare against i.e. highest possible date that it supports.

squat · 2020-06-25T13:53:42Z

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx

+                    {minTime >= MAX_TIME ? <FontAwesomeIcon icon={faInfinity} /> : formatTime(minTime)}
+                  </td>
+                  <td data-testid="maxTime">
+                    {maxTime >= MAX_TIME ? <FontAwesomeIcon icon={faInfinity} /> : formatTime(maxTime)}


* upstream/release-0.14: (46 commits) Cut release v0.14.0-rc.1 (thanos-io#2853) Query: correctly marshal errors to JSON and ignore if nil (thanos-io#2848) ci: Manually download promu in crossbuild stage (thanos-io#2828) Cut release v0.14.0-rc.0 (thanos-io#2826) Soft cut changelog on master to indicate v0.14.0 being in progress (thanos-io#2824) Update ThanosReceiveNoUpload to select sum == 0 (thanos-io#2819) receive: Added more observability, fixed leaktest, to actually check leaks ): (thanos-io#2817) Query: always return a string in the `lastError` field (thanos-io#2809) Added missing CHANGELOG entry for PR 2613 (thanos-io#2820) receive: Fixed small options race; Removed unused StartTime feature. (thanos-io#2816) go.mod: Bump Prometheus to current latest (thanos-io#2814) Implement CLI Flags page in React UI (thanos-io#2796) Improve ThanosReceiveNoUpload to only alert on current instances store: Preallocate output buffer when encoding postings. (thanos-io#2812) compact: introduce flag --block-viewer.global.sync-block-interval (thanos-io#2752) docs: compact: add blurb about how retention policy works (thanos-io#2808) Reduced memory allocations in readIndexRange() (thanos-io#2807) ui: Add Stores page to React UI (thanos-io#2754) Added Kemal to Maintainer Role; Kemal is volounteering to be next release shephard (thanos-io#2804) proposal: Add scalable rule storage proposal (thanos-io#2661) ...

onprem added 2 commits June 12, 2020 02:40

ui: Add store page to React UI

63e7e09

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

query: Remove unnecessary field 'store_type' from api response

78b600e

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

soniasingla reviewed Jun 12, 2020

View reviewed changes

GiedriusS reviewed Jun 12, 2020

View reviewed changes

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx Outdated Show resolved Hide resolved

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx Outdated Show resolved Hide resolved

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx Outdated Show resolved Hide resolved

ui: React: Show infinity icon the time is too big

5273fa5

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

GiedriusS reviewed Jun 14, 2020

View reviewed changes

GiedriusS reviewed Jun 15, 2020

View reviewed changes

pkg/ui/react-app/src/thanos/pages/stores/StorePoolPanel.tsx Show resolved Hide resolved

onprem added 2 commits June 17, 2020 02:04

ui: Add tests for the Stores page in React UI

16a9ce3

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

Merge branch 'master' into react-store-page

301f8d7

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

onprem mentioned this pull request Jun 18, 2020

Action Plan for Thanos User Interface and UI related APIs Improvements #2198

Closed

13 tasks

Merge branch 'master' into react-store-page

623a8e1

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

GiedriusS approved these changes Jun 25, 2020

View reviewed changes

GiedriusS reviewed Jun 25, 2020

View reviewed changes

squat approved these changes Jun 25, 2020

View reviewed changes

GiedriusS merged commit 1aca077 into thanos-io:master Jun 25, 2020

onprem deleted the react-store-page branch June 25, 2020 16:04

Conversation

onprem commented Jun 12, 2020

Changes

Verification

Uh oh!

soniasingla left a comment

Choose a reason for hiding this comment

Uh oh!

GiedriusS commented Jun 12, 2020

Uh oh!

GiedriusS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

onprem commented Jun 12, 2020

Uh oh!

GiedriusS left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onprem commented Jun 14, 2020

Uh oh!

GiedriusS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GiedriusS left a comment

Choose a reason for hiding this comment

Uh oh!

GiedriusS Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

squat Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

squat Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

GiedriusS Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

squat Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

GiedriusS Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

squat Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GiedriusS left a comment •

edited

Loading