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

Optimize Target, Alert and Service Discovery pages #5119

Merged
merged 12 commits into from
Feb 10, 2022

Conversation

Nexucis
Copy link
Contributor

@Nexucis Nexucis commented Feb 3, 2022

This PR is following the enhancements of the Alert, Target and Service discovery pages from Prometheus.

It also align the Target Page regarding the way to manage the filter. It was easier like that to apply the changes.

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

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 3, 2022

@onprem I supposed this one is for you.
I still have a test that failed, but I'm not really sure why it is failing. The number of ScrapePoolPanel should remain the same :/

@GiedriusS
Copy link
Member

GiedriusS commented Feb 4, 2022

It seems like the CI is failing at the linting stage:

/home/runner/work/thanos/thanos/pkg/ui/react-app/src/pages/targets/Filter.test.tsx
Error:    23:7   error  Delete `··`                                    prettier/prettier
Error:    92:11  error  Delete `··`                                    prettier/prettier
Error:   102:4   error  Newline required at end of file but not found  eol-last
Error:   102:4   error  Insert `⏎`                                     prettier/prettier

✖ 4 problems (4 errors, 0 warnings)
  4 errors and 0 warnings potentially fixable with the `--fix` option.

I think the linter is only suggesting cosmetic changes? 🤔 This is weird since there are no such things in the code 🤔

@GiedriusS
Copy link
Member

GiedriusS commented Feb 4, 2022

--fix suggests:

diff --git a/pkg/ui/react-app/src/pages/targets/Filter.test.tsx b/pkg/ui/react-app/src/pages/targets/Filter.test.tsx
index d3c20f6c..620866f7 100644
--- a/pkg/ui/react-app/src/pages/targets/Filter.test.tsx
+++ b/pkg/ui/react-app/src/pages/targets/Filter.test.tsx
@@ -20,7 +20,7 @@ describe('Filter', () => {
     setFilter = sinon.spy();
     setExpaned = sinon.spy();
     filterWrapper = shallow(
-        <Filter filter={initialState} setFilter={setFilter} expanded={initialExpanded} setExpanded={setExpaned} />
+      <Filter filter={initialState} setFilter={setFilter} expanded={initialExpanded} setExpanded={setExpaned} />
     );
   });
 
@@ -89,7 +89,7 @@ describe('Filter', () => {
         const filterCallback = sinon.spy();
         const expandedCallback = sinon.spy();
         const filterW = shallow(
-            <Filter filter={filter} setFilter={filterCallback} expanded={initial} setExpanded={expandedCallback} />
+          <Filter filter={filter} setFilter={filterCallback} expanded={initial} setExpanded={expandedCallback} />
         );
         const btn = filterW.find(Button).filterWhere((btn): boolean => btn.hasClass('expansion'));
         expect(btn.children().text()).toEqual(text);
@@ -99,4 +99,4 @@ describe('Filter', () => {
       });
     });
   });
-});
\ No newline at end of file
+});
diff --git a/pkg/ui/react-app/src/thanos/pages/blocks/Blocks.tsx b/pkg/ui/react-app/src/thanos/pages/blocks/Blocks.tsx
index c488e6b5..1b7512c4 100644
--- a/pkg/ui/react-app/src/thanos/pages/blocks/Blocks.tsx
+++ b/pkg/ui/react-app/src/thanos/pages/blocks/Blocks.tsx
@@ -89,7 +89,7 @@ export const BlocksContent: FC<{ data: BlockListProps }> = ({ data }) => {
   const onChangeCompactionCheckbox = (target: EventTarget & HTMLInputElement) => {
     setFilterCompaction(target.checked);
     if (target.checked) {
-      let compactionLevel: number = parseInt(compactionLevelInput);
+      const compactionLevel: number = parseInt(compactionLevelInput);
       setQuery({
         'filter-compaction': target.checked,
         'compaction-level': compactionLevel,

Test failure looks like:

FAIL src/pages/targets/ScrapePoolList.test.tsx
  ● ScrapePoolList › when data is returned › renders a table

    expect(received).toHaveLength(expected)

    Expected length: 3
    Received length: 0
    Received object: {}

      42 |       expect(mock).toHaveBeenCalledWith('../api/v1/targets?state=active', { cache: 'no-store', credentials: 'same-origin' });
      43 |       const panels = scrapePoolList.find(ScrapePoolPanel);
    > 44 |       expect(panels).toHaveLength(3);
         |                      ^
      45 |       const activeTargets: Target[] = sampleApiResponse.data.activeTargets as unknown as Target[];
      46 |       activeTargets.forEach(({ scrapePool }: Target) => {
      47 |         const panel = scrapePoolList.find(ScrapePoolPanel).filterWhere((panel) => panel.prop('scrapePool') === scrapePool);

      at Object.<anonymous> (src/pages/targets/ScrapePoolList.test.tsx:44:22)

@Nexucis does the patch look good? 🤔

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 4, 2022

aah I didn't see the linter issue. Thanks @GiedriusS for the highlight :).
For the test issue, I still don't have a clue on why it's failing sorry :(.

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 7, 2022

Lol turns out I forgot a modification in the target page. So it was good the test failed \o/.
It's good now @GiedriusS.
Let me know if you guys prefer I'm cutting this PR into multiple one. I did it like that because the changes are more or less a report / sync from what we did in Prometheus.

@wiardvanrij
Copy link
Member

I don't think thats necessary. I've checked the PR's from Prometheus and it seems like it had a solid review there :D Did you test these changes for Thanos itself (i.e. perhaps we have something specific)?

My only nit would be the changelog, the sentence is a bit odd, (maybe it had to be "add a searchbar?") -

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.

Don't forget make assets (:

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 7, 2022

Don't forget make assets (:

Ah yeah thanks for the remember, I totally forgot thanks :).

I don't think thats necessary. I've checked the PR's from Prometheus and it seems like it had a solid review there :D

Cool thanks !

Did you test these changes for Thanos itself (i.e. perhaps we have something specific)?

I tested in Thanos yes, but not super heavily (just to be sure I didn't forgot something).

My only nit would be the changelog, the sentence is a bit odd, (maybe it had to be "add a searchbar?") -

Ah yup I forgot a word there. Sorry about that and thanks for your eyes :) !

GiedriusS
GiedriusS previously approved these changes Feb 7, 2022
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.

Thank you, looks amazing! Tested on my cluster.

@GiedriusS
Copy link
Member

GiedriusS commented Feb 7, 2022

Search doesn't do anything, though:

main.41b953b1.chunk.js:1 Uncaught TypeError: Cannot read properties of null (reading 'value')
    at handleChange (main.41b953b1.chunk.js:1)
    at main.41b953b1.chunk.js:1

@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 7, 2022

:o that's weird, on which page @GiedriusS ?

@GiedriusS
Copy link
Member

GiedriusS commented Feb 8, 2022

/alerts. If I enter anything this is what I get:

AlertContents.tsx:81 Uncaught TypeError: Cannot read properties of null (reading 'value')
    at handleSearchChange (AlertContents.tsx:81)
    at SearchBar.tsx:17

The backtrace:

  14 | const handleSearchChange = (e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>) => {
  15 |   clearTimeout(filterTimeout);
  16 |   filterTimeout = setTimeout(() => {
> 17 |    handleChange(e);
^  18 |   }, 300);
  19 | };
  20 |
  78 | };
  79 | 
  80 | const handleSearchChange = (e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>) => {
> 81 |   if (e.target.value !== '') {
^  82 |    const pattern = e.target.value.trim();
  83 |     const result: RuleGroup[] = [];
  84 |     for (const group of groups) {

e.target is null for some reason?

There's also this message in the logs pointing to https://reactjs.org/docs/legacy-event-pooling.html. Do we need to call .persist() or update React to v17? Maybe we can update to v17 React in this same PR?

@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 8, 2022

aaah thanos is still in React 16, which is not the case for Prometheus.
Thank you so much for the investigation @GiedriusS !
Unfortunately upgrading to React 17 is something that cannot be done in this PR, it is a bigger PR.

I'm wondering if we should first upgrade to React 17 and put on hold this one, or do what you suggested and then make the upgrade to React 17. WDYT ?

@GiedriusS
Copy link
Member

Maybe let's add e.persist() and then upgrade React in a separate PR?

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 8, 2022

alright, it should work better like that now @GiedriusS. Thanks again for the help !

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 you forgot make assets 😄

@GiedriusS
Copy link
Member

And you forgot one persist call in another handleSearchChange

@@ -54,49 +77,80 @@ const AlertsContent: FC<AlertsProps> = ({ groups = [], statsCount }) => {
});
};

const handleSearchChange = (e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>) => {
Copy link
Member

Choose a reason for hiding this comment

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

persist() missing (:

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 put it in the global component SearchBar (see 46031b7).

Hopefully that's the only place it's required. The function you are looking here is called by the search component where you have the debounce mechanism. So it's in this component that you should persist the event

On my side it's working, is it still broken on your side @GiedriusS

const [targetList, setTargetList] = useState(processSummary(activeTargets, droppedTargets));
const [labelList, setLabelList] = useState(processTargets(activeTargets, droppedTargets));

const handleSearchChange = (e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>) => {
Copy link
Member

Choose a reason for hiding this comment

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

persist() missing (:

const { showHealthy, showUnhealthy } = filter;

const handleSearchChange = (e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>) => {
Copy link
Member

Choose a reason for hiding this comment

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

persist() missing (:

@Nexucis
Copy link
Contributor Author

Nexucis commented Feb 8, 2022

I think you forgot make assets 😄

Ah yeah indeed :(. It would be cool, if this thing would be generated in the CI instead like it is the case in Prometheus.

Signed-off-by: Augustin Husson <husson.augustin@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.

LGTM. Thanks for this!

@onprem onprem merged commit b2c1ff0 into thanos-io:main Feb 10, 2022
@Nexucis Nexucis deleted the feature/rework-page branch February 10, 2022 21:38
Nicholaswang pushed a commit to Nicholaswang/thanos that referenced this pull request Mar 6, 2022
* add generic component for infinite scroll and search bar

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* rework service discovery page

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* rework alert page

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* rework target page

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* add entry in the changelog

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* fix test

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* fix linter issue

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* add infinite scroll on target

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* modify the assets

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* fix Changelog

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* persis the search event

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

* add missing bindata

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>
Signed-off-by: Nicholaswang <wzhever@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants