-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Signed-off-by: Augustin Husson <[email protected]>
Signed-off-by: Augustin Husson <[email protected]>
Signed-off-by: Augustin Husson <[email protected]>
Signed-off-by: Augustin Husson <[email protected]>
Signed-off-by: Augustin Husson <[email protected]>
Signed-off-by: Augustin Husson <[email protected]>
@onprem I supposed this one is for you. |
It seems like the CI is failing at the linting stage:
I think the linter is only suggesting cosmetic changes? 🤔 This is weird since there are no such things in the code 🤔 |
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:
@Nexucis does the patch look good? 🤔 |
Signed-off-by: Augustin Husson <[email protected]>
aah I didn't see the linter issue. Thanks @GiedriusS for the highlight :). |
Signed-off-by: Augustin Husson <[email protected]>
cd877ea
to
f0acd64
Compare
Lol turns out I forgot a modification in the target page. So it was good the test failed \o/. |
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?") - |
There was a problem hiding this 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 <[email protected]>
Signed-off-by: Augustin Husson <[email protected]>
Ah yeah thanks for the remember, I totally forgot thanks :).
Cool thanks !
I tested in Thanos yes, but not super heavily (just to be sure I didn't forgot something).
Ah yup I forgot a word there. Sorry about that and thanks for your eyes :) ! |
There was a problem hiding this 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.
Search doesn't do anything, though:
|
:o that's weird, on which page @GiedriusS ? |
The backtrace:
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? |
aaah thanos is still in React 16, which is not the case for Prometheus. 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 ? |
Maybe let's add |
Signed-off-by: Augustin Husson <[email protected]>
alright, it should work better like that now @GiedriusS. Thanks again for the help ! |
There was a problem hiding this 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
😄
And you forgot one |
@@ -54,49 +77,80 @@ const AlertsContent: FC<AlertsProps> = ({ groups = [], statsCount }) => { | |||
}); | |||
}; | |||
|
|||
const handleSearchChange = (e: ChangeEvent<HTMLTextAreaElement | HTMLInputElement>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
persist()
missing (:
There was a problem hiding this comment.
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>) => { |
There was a problem hiding this comment.
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>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
persist()
missing (:
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 <[email protected]>
There was a problem hiding this 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!
* add generic component for infinite scroll and search bar Signed-off-by: Augustin Husson <[email protected]> * rework service discovery page Signed-off-by: Augustin Husson <[email protected]> * rework alert page Signed-off-by: Augustin Husson <[email protected]> * rework target page Signed-off-by: Augustin Husson <[email protected]> * add entry in the changelog Signed-off-by: Augustin Husson <[email protected]> * fix test Signed-off-by: Augustin Husson <[email protected]> * fix linter issue Signed-off-by: Augustin Husson <[email protected]> * add infinite scroll on target Signed-off-by: Augustin Husson <[email protected]> * modify the assets Signed-off-by: Augustin Husson <[email protected]> * fix Changelog Signed-off-by: Augustin Husson <[email protected]> * persis the search event Signed-off-by: Augustin Husson <[email protected]> * add missing bindata Signed-off-by: Augustin Husson <[email protected]> Signed-off-by: Nicholaswang <[email protected]>
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.