-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat: Enable grouping of same agents in the status overview from the Configuration UI #1444
Conversation
0ded8cd
to
0ff8b7b
Compare
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.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Napfschnecke)
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js
line 135 at r2 (raw file):
} `}</style> {name} {agentIdElement} {rowData.count > 1 ? <b>({rowData.count})</b> : null}
Can you show this information as a.. how is it called.. label/tag/badge. I mean something like this: https://www.primefaces.org/primereact/badge/ or actually like this: https://www.primefaces.org/primereact/tag/ (keep in mind that we don't have the latest version of this ui framework, so these elements might not exist)
So that it is not just the bold number (I know I added a screenshot of a bold number to the ticket :) )
Code quote:
<b>({rowData.count})</b>
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusToolbar.js
line 92 at r2 (raw file):
</div> <div className="p-inputgroup checkbox-group"> <label>Merge Identical Services</label>
How about naming this Combine duplicate services
or Hide duplicate services
? Imo "merge" feels wrong for this element.
And can you also add some sort of "?"-Icon, showing a tooltip that explins how this feature actually works: so that a duplicate is considered if it hase the same service name.
Code quote:
Merge Identical Services
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js
line 22 at r2 (raw file):
filter: '', useRegexFilter: false, useServiceMerge: false,
Please set this to true
by default.
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js
line 44 at r2 (raw file):
onServiceMergeChange = ({ checked }) => { this.setState({ useServiceMerge: checked }, this.filterAgents);
It should be fine directly calling mergeAgents
because the filter value is not changing, so we don't have to filter the agents again.
Code quote:
this.filterAgents
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js
line 115 at r2 (raw file):
mergeAgents = () => { const { filteredAgents, useServiceMerge } = this.state;
Since this attribute is not only filtered but also "merged" now, imo we should rename it. Maybe something like agentsToShow
or something in that direction... What do you think?
Code quote:
filteredAgents
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js
line 120 at r2 (raw file):
const mergedMap = new Map(); for (const agent of filteredAgents) {
The reduce
function would also be a good way to build this map. So instead of the loop, you can do the following:
const merged2 = filteredAgents.reduce((result, agent) => {
const name = this.getServiceName(agent);
if (!!result[name]) {
if (this.getStartTime(result[name]) < this.getStartTime(agent)) {
result[name] = {
...agent,
count: result[name].count + 1
};
}
} else {
result[name] = {
...agent,
count: 1
};
}
return result;
}, {});
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js
line 122 at r2 (raw file):
for (const agent of filteredAgents) { if (!mergedMap.has(this.getServiceName(agent))) { mergedMap.set(this.getServiceName(agent), {
Can you extract the service name into a local vairable? This will make the code easier to read instead having getServiceName
all over the place :)
Code quote:
this.getServiceName(agent)
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.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mariusoe and @Napfschnecke)
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js
line 135 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can you show this information as a.. how is it called.. label/tag/badge. I mean something like this: https://www.primefaces.org/primereact/badge/ or actually like this: https://www.primefaces.org/primereact/tag/ (keep in mind that we don't have the latest version of this ui framework, so these elements might not exist)
So that it is not just the bold number (I know I added a screenshot of a bold number to the ticket :) )
Something like this? Open to suggestions on color, corners, etc. :)
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusToolbar.js
line 92 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
How about naming this
Combine duplicate services
orHide duplicate services
? Imo "merge" feels wrong for this element.And can you also add some sort of "?"-Icon, showing a tooltip that explins how this feature actually works: so that a duplicate is considered if it hase the same service name.
Again, open to suggestions for the design :)
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js
line 44 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
It should be fine directly calling
mergeAgents
because the filter value is not changing, so we don't have to filter the agents again.
If I call mergeAgents directly it won't un-merge on deselection of the checkbox. The list after merging overwrites the filtered list, so I need to reapply the filter upon deselecting the merge-feature to get the squished agents back. I could save the filtered agent list in a separate variable that I can reset to, if thats prefered :)
… the Configuration UI
0ff8b7b
to
8811953
Compare
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.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Napfschnecke)
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js
line 135 at r2 (raw file):
Previously, Napfschnecke (sbr) wrote…
Something like this? Open to suggestions on color, corners, etc. :)
Yes, thats what I meant!
For me, you can keep it that way.
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js
line 148 at r3 (raw file):
{rowData.count > 1 ? ( <span className="badge"> <b>{rowData.count}</b>{' '}
Can be removed, I think. Or do we need this space?
Code quote:
{' '}
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusToolbar.js
line 88 at r3 (raw file):
color: white; } &.__react_component_tooltip.show :global(*) {
Can this be a problem with different tooltips? As far as I understand it, this would affect all tooltips. Or am I wrong? I don't fully understand the &.
notation :)
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusView.js
line 44 at r2 (raw file):
Previously, Napfschnecke (sbr) wrote…
If I call mergeAgents directly it won't un-merge on deselection of the checkbox. The list after merging overwrites the filtered list, so I need to reapply the filter upon deselecting the merge-feature to get the squished agents back. I could save the filtered agent list in a separate variable that I can reset to, if thats prefered :)
Ah, good point.
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mariusoe and @Napfschnecke)
components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusToolbar.js
line 88 at r3 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can this be a problem with different tooltips? As far as I understand it, this would affect all tooltips. Or am I wrong? I don't fully understand the
&.
notation :)
This would, presumably, affect all places where the ReactTooltip is used. Vast majority of our tooltips seem to be on Buttons with the tooltip prop or spans using the title prop. Only other usage of ReactTooltip is in the rule documentation.
I only added that style override because the semi-translucent tooltip was a bit annoying to read with more text in it.
I can revert it or just use the normal span-title tooltip if you want to keep it more consistent with the rest of the app.
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Napfschnecke)
Closes #1395
Added a new checkbox to the toolbar of the Agent Status view, which enables merging list entries with the same service-name into a single entry with an indicator for the number of merged agents.
This change is