remove unnecessary metadata propagation #3007
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were setting the very same k/v that we just looked up.
instead of the equivalent long-hand code.
The 'create' function passed to addChild is only ever invoked when we cannot find a matching process in the process topology. In these cases the host and pid will be _all_ the info we are ever going to have, both of which are incorporated in the id. Node summarisation renders that as best it can; adding the PID as metadata does not make a difference but for a line with that info in the detail panel, which adds nothing since the PID is already included in the label.
The name is already the id of the node, and that's what summarisation renders. Nothing looks at process.Name.
The HostNodeID is already the id of host nodes (as the name suggests), and that's what summarisation renders. Nothing looks at the HostNodeID metadata of host nodes.
The container hostname is already the id of the node, and that's what summarisation renders. Nothing looks at the docker.ContainerHostname metadata of nodes in the ContainerHostname group topology.
This was building a set of all the image ids represented by the same unversioned image. Well, it was doing that until I broke it with a silly mistake in #1739 - instead of extracting the imageID from the original node ID, it's extracting it from the updated ID, which is the unversioned image. Even if it was working though, it's pointless since nothing is looking at that info.
The ImageID is already the id of the node we are creation, and that's what summarisation renders in the event we fail to join this node with a node from the ContainerImage topology that has more metadata. Nothing is looking at the ImageID metadata field.
...when creating Uncontained pseudo nodes. Summarisation of Uncontained/Umanaged only looks at the ID, which includes the HostNodeID. We adjust the promotion of Uncontained to Unmanaged, to operate on the ID instead of (re)extracting the hostID from the HostNodeID metadata. With that, nothing looks at the HostNodeID metadata of Uncontained/Unmanaged nodes.
...when creating Uncontained pseudo nodes. IsConnectedMark only affects summarisation of genuine process nodes.
rade
force-pushed
the
less-derived-node-metadata
branch
3 times, most recently
from
aa5be8d to
5159ba1
Compare
|
I think the last commit message could be improved.
I would phrase it more like "after dropping extra metadata in the rest of this PR, our usage of ..." |
bboreham
approved these changes
Dec 29, 2017
render/container.go
Outdated
| if found && id != "" { // not one we blanked out earlier | ||
| // We are guaranteed to find the id, so no need to pass a node constructor. | ||
| ret.addChild(m, id, nil) | ||
| if found && id != "" { // not one we blanked out earlier We |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
render/container.go
Outdated
| // an per-host "Uncontained" node. If for whatever reason this | ||
| // node doesn't have a host id in their node metadata, it'll all | ||
| // get grouped into a single uncontained node. | ||
| // an per-host "Uncontained" node. |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Some process nodes may not have a HostNodeID metadata, e.g. when an endpoint references a pid that we know nothing about. When mapping processes to containers, we therefore shouldn't rely on HostNodeID. Instead we can obtain the hostID from the process node ID. This has been broken for a while, possibly forever.
After dropping extra metadata in the rest of this PR, our usage of joinResults.add* only ever ends creating minimal nodes, from just an id and topology. Hence joinResults.add* can be invoked with simply an id and topology instead of a generic node creation function.
Fixed. |
rade
force-pushed
the
less-derived-node-metadata
branch
from
5159ba1 to
c8ea7ba
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turns out that nothing is actually looking at the metadata fields we were propagating in the Map/join functions. Instead, all the metadata being rendered comes from nodes we are joining with. Furthermore, summarisation does a reasonable job of rendering nodes knowing just their ID and topology.
This change makes the Map/join functions much more regular. With the exception of
PropagateSingleMetric(which really shouldn't be a Map - see #3008), all these functions now produce nodes with just an id & topology, and no metadata. This allows us to simplyjoinResults.add*. It also brings some performance benefits - a few % saved allocations in the rendering benchmarks. And it's a step on the way to unifying Map and join.