-
Notifications
You must be signed in to change notification settings - Fork 897
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
Refresh graph logging: use graphviz syntax #15814
Conversation
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 must have 👍
This was somewhere in the depths of my head, but seeing this now, it's so awesome!!! :-) 🥇 @agrare what do you think, does it make sense to have it here, or in the tools for processing logs? Also I am not sure, can we atomically append multiline strings to log? (I was thinking that we can't, so I wanted to rewrite the logging a bit) |
@cben really, really cool!! /cc @blomquisg @Fryguy |
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.
This is a great idea @cben :) Just a couple of nits
We have a few log_processing
tools, maybe you can add that command to build the image here?
https://github.com/ManageIQ/manageiq/tree/master/tools/log_processing
app/models/manager_refresh/graph.rb
Outdated
node_names = {} | ||
# Want readable ids, but can't have duplicates, so append numbers. | ||
# Try to use shorter .name method that InventoryCollection has. | ||
nodes.group_by {|n| n.name.to_s rescue n.to_s }.each do |base_name, ns| |
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.
group_by {|n|
Can you add a space between your {|n|
?
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.
n.name.to_s rescue n.to_s
Not crazy about this rescue, how about a node_friendly_name
helper which checks if n.kind_of? InventoryCollection
or n.respond_to? :name
?
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.
we can probably just assign nicer names to ICs :-)
app/models/manager_refresh/graph.rb
Outdated
@@ -10,6 +10,38 @@ def initialize(nodes) | |||
construct_graph!(@nodes) | |||
end | |||
|
|||
def to_graphviz(layers: nil) | |||
node_names = {} |
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 was expecting a require "graphviz"
or something here, but don't see it.
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.
Not running the render at logging time, just printing text that's valid graphviz syntax.
I suppose there are gems that generate graphviz syntax, but can't make it much simpler, plus here I don't just want any valid syntax, I want particular that's also readable to a human just reading the log.
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.
Ahhh...makes sense.
Also, I agree that this belongs in the tools subdirectory somewhere. |
Added spec, short script under tools/, PTAL |
Checked commit cben@fb6732f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
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.
Tested it out, definitely more readable 👍
I felt like doing something silly yesterday evening. Now I finally have something pretty to show for
myLadislav's efforts :-)After we DAGify and topologically sort the inventory graph into layers, we currently dump it into evm.log in an ad-hoc format:
This PR instead dumps it in GraphViz compatible syntax, which I tried to keep a bit shorter and at least as readable:
but then can be pasted into (thanks to @anpc and @yaacov for some fine tuning ;-))
producing the above image: 🎉
https://gist.github.com/cben/ba81cd462484664986ae410df27fd67a has all fulls texts with images.
The function tries to use
InventoryCollection#name
but technically in this class shouldn't assume that so has a fallback to.to_s
. Above gist has example of output it would do if.name
wasn't available:The long node names are properly quoted and work, just make it look ugly.
The function supports just dumping a graph, without layers. Again, above gist has example, output here. (dot's ranking algorithm just happens to lay them out in similar layers.)
@miq-bot add-label developer, enhancement
cc @Ladas @agrare what do you think?
probably deserves a test. perhaps it can also be handy in other tests?