-
Notifications
You must be signed in to change notification settings - Fork 712
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
Simplify some renderers #3747
Simplify some renderers #3747
Conversation
Crunch a Map and Reduce into a single Renderer that has the same function.
The second half of the render added all the same nodes, except for those with no Docker image information, so we could show a figure for those filtered out on that basis. This just isn't worth the effort.
ContainerWithImageNameRenderer, | ||
), | ||
// Grab *all* the hostnames, so we can count the number which were empty | ||
// for accurate stats. |
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.
Where would these stats be displayed?
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.
Bottom-left of the screen, like "21 nodes (119 filtered)"
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.
Ok cool. That information's been misleading and incorrect at times for container topologies so I'm ok seeing it removed.
Looking at the UI code, it seems it's already robust enough to not display filtered nodes count if it's not provided so probably nothing will need to be updated there 👍
// Grab *all* the hostnames, so we can count the number which were empty | ||
// for accurate stats. | ||
MakeMap( | ||
MapToEmpty, |
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.
After removing it from here, this method is not used anymore so feel free to remove it from container.go
.
// ContainerImageRenderer is a Renderer which produces a renderable container | ||
// image graph by merging the container graph and the container image topology. | ||
// ContainerImageRenderer produces a graph where each node is a container image | ||
// with the original containers as children |
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.
👍 Thanks for clarity!
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.
Nice one! 👌
Left only some minor comments, it seems that the test coverage for this piece of logic is already decent, did you also do some manual testing?
Do you expect a significant performance improvement or was this PR more about simplifying the code?
I am yak-shaving towards a significant performance improvement; this one piece doesn't do much but it helps to simplify the problem. |
ContainerImageRenderer
: Crunch aMap
andReduce
into a singleRenderer
that has the same function.ContainerHostnameRenderer
: The second half of the render added all the same nodes, except forthose with no Docker image information, so we could show a figure for those filtered out on that basis. This just isn't worth the effort.
And remove unused parameterisation of
containerWithImageNameRenderer