From 1ce1ecad7c19ee39f8445b6e55fd7d625f344c0f Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 10 Mar 2020 11:30:25 +0000 Subject: [PATCH 1/4] In unit tests, check counters --- render/expected/expected.go | 21 ++++++++++++++++++++- test/utils/prune.go | 4 +++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/render/expected/expected.go b/render/expected/expected.go index f5f478de0..fcf584838 100644 --- a/render/expected/expected.go +++ b/render/expected/expected.go @@ -140,6 +140,7 @@ var ( )), fixture.NonContainerName: processNameNode(fixture.NonContainerName, render.OutgoingInternetID). + WithCounters(map[string]int{report.Process: 1}). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], @@ -205,6 +206,7 @@ var ( WithLatests(map[string]string{ docker.ContainerHostname: fixture.ServerContainerHostname, }). + WithCounters(map[string]int{report.Container: 1}). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], @@ -238,6 +240,7 @@ var ( )), ServerContainerImageNodeID: containerImage(ServerContainerImageNodeID). + WithCounters(map[string]int{report.Container: 1}). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], @@ -254,10 +257,12 @@ var ( uncontainedServerNode, RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], - )) + )). + WithCounters(map[string]int{render.Pseudo: 1}) RenderedPods = report.Nodes{ fixture.ClientPodNodeID: pod(fixture.ClientPodNodeID, fixture.ServerPodNodeID). + WithCounters(map[string]int{report.Container: 1}). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -267,6 +272,7 @@ var ( )), fixture.ServerPodNodeID: pod(fixture.ServerPodNodeID). + WithCounters(map[string]int{report.Container: 1}). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], @@ -323,6 +329,7 @@ var ( RenderedPodServices = report.Nodes{ fixture.ServiceNodeID: service(fixture.ServiceNodeID, fixture.ServiceNodeID). + WithCounters(map[string]int{report.Pod: 2}). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -346,6 +353,12 @@ var ( WithLatests(map[string]string{ report.HostName: fixture.ClientHostName, }). + WithCounters(map[string]int{ + report.Container: 1, + report.ContainerImage: 1, + report.Pod: 1, + report.Process: 2, + }). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -357,6 +370,12 @@ var ( )), fixture.ServerHostNodeID: hostNode(fixture.ServerHostNodeID, render.OutgoingInternetID). + WithCounters(map[string]int{ + report.Container: 1, + report.ContainerImage: 1, + report.Pod: 1, + report.Process: 2, + }). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedEndpoints[fixture.NonContainerNodeID], diff --git a/test/utils/prune.go b/test/utils/prune.go index 42a8aa268..0e1affc16 100644 --- a/test/utils/prune.go +++ b/test/utils/prune.go @@ -22,9 +22,11 @@ func PruneNode(node report.Node) report.Node { node.Children.ForEach(func(child report.Node) { prunedChildren = prunedChildren.Add(PruneNode(child)) }) - return report.MakeNode( + prunedNode := report.MakeNode( node.ID). WithTopology(node.Topology). WithAdjacent(node.Adjacency...). WithChildren(prunedChildren) + prunedNode.Counters = node.Counters + return prunedNode } From c03aeb5d437fbafb023c7ad94be77706ef6fb83c Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sun, 20 Oct 2019 10:16:36 +0000 Subject: [PATCH 2/4] Move Counters into Latest The only place Counters are used is in rendering, for the number of nodes under a topology, so the overhead of holding a unique data structure in every Node is unwarranted. Counters are not set in the probe, so we don't need any backwards-compatibility in report decoding. Similarly they are not set until after all nodes are merged, so we don't need that logic. --- render/container.go | 4 +- render/detailed/summary.go | 14 +-- render/expected/expected.go | 46 ++++------ render/render.go | 2 +- report/backcompat.go | 5 + report/counters.go | 137 ---------------------------- report/counters_internal_test.go | 152 ------------------------------- report/map_keys.go | 1 + report/metadata_template.go | 2 +- report/node.go | 27 ++++-- report/node_test.go | 52 ++++++----- report/report_test.go | 6 +- test/utils/prune.go | 10 +- 13 files changed, 95 insertions(+), 363 deletions(-) delete mode 100644 report/counters.go delete mode 100644 report/counters_internal_test.go diff --git a/render/container.go b/render/container.go index f40ca8227..da3042c84 100644 --- a/render/container.go +++ b/render/container.go @@ -326,7 +326,7 @@ func MapContainer2Hostname(n report.Node) report.Node { return report.Node{} } - node := NewDerivedNode(id, n).WithTopology(containerHostnameTopology) - node.Counters = node.Counters.Add(n.Topology, 1) + node := NewDerivedNode(id, n).WithTopology(containerHostnameTopology). + AddCounter(n.Topology, 1) return node } diff --git a/render/detailed/summary.go b/render/detailed/summary.go index 6f42f79f8..e18cefd5a 100644 --- a/render/detailed/summary.go +++ b/render/detailed/summary.go @@ -164,7 +164,7 @@ func MakeNodeSummary(rc RenderContext, n report.Node) (NodeSummary, bool) { Adjacency: n.Adjacency, } // Only include metadata, metrics, tables when it's not a group node - if _, ok := n.Counters.Lookup(n.Topology); !ok { + if _, ok := n.LookupCounter(n.Topology); !ok { if topology, ok := rc.Topology(n.Topology); ok { summary.Metadata = topology.MetadataTemplates.MetadataRows(n) summary.Metrics = topology.MetricTemplates.MetricRows(n) @@ -292,7 +292,7 @@ func containerImageNodeSummary(base BasicNodeSummary, n report.Node) BasicNodeSu // heuristic regexp match we cannot tell the difference. base.Label, _ = report.ParseContainerImageNodeID(n.ID) } - base.LabelMinor = pluralize(n.Counters, report.Container, "container", "containers") + base.LabelMinor = pluralize(n, report.Container, "container", "containers") base.Rank = base.Label base.Stack = true return base @@ -314,7 +314,7 @@ func addKubernetesLabelAndRank(base BasicNodeSummary, n report.Node) BasicNodeSu func podNodeSummary(base BasicNodeSummary, n report.Node) BasicNodeSummary { base = addKubernetesLabelAndRank(base, n) - base.LabelMinor = pluralize(n.Counters, report.Container, "container", "containers") + base.LabelMinor = pluralize(n, report.Container, "container", "containers") return base } @@ -331,7 +331,7 @@ func podGroupNodeSummary(base BasicNodeSummary, n report.Node) BasicNodeSummary base.Stack = true // NB: pods are the highest aggregation level for which we display // counts. - count := pluralize(n.Counters, report.Pod, "pod", "pods") + count := pluralize(n, report.Pod, "pod", "pods") if typeName, ok := podGroupNodeTypeName[n.Topology]; ok { base.LabelMinor = fmt.Sprintf("%s of %s", typeName, count) } else { @@ -423,7 +423,7 @@ func groupNodeSummary(base BasicNodeSummary, r report.Report, n report.Node) Bas base.Shape = t.GetShape() base.Tag = t.Tag if t.Label != "" { - base.LabelMinor = pluralize(n.Counters, topology, t.Label, t.LabelPlural) + base.LabelMinor = pluralize(n, topology, t.Label, t.LabelPlural) } } } @@ -431,8 +431,8 @@ func groupNodeSummary(base BasicNodeSummary, r report.Report, n report.Node) Bas return base } -func pluralize(counters report.Counters, key, singular, plural string) string { - c, ok := counters.Lookup(key) +func pluralize(n report.Node, key, singular, plural string) string { + c, ok := n.LookupCounter(key) if !ok { c = 0 } diff --git a/render/expected/expected.go b/render/expected/expected.go index fcf584838..b11137f2f 100644 --- a/render/expected/expected.go +++ b/render/expected/expected.go @@ -123,7 +123,7 @@ var ( RenderedProcessNames = report.Nodes{ fixture.Client1Name: processNameNode(fixture.Client1Name, fixture.ServerName). WithLatests(map[string]string{process.Name: fixture.Client1Name}). - WithCounters(map[string]int{report.Process: 2}). + AddCounter(report.Process, 2). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -133,14 +133,14 @@ var ( fixture.ServerName: processNameNode(fixture.ServerName). WithLatests(map[string]string{process.Name: fixture.ServerName}). - WithCounters(map[string]int{report.Process: 1}). + AddCounter(report.Process, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], )), fixture.NonContainerName: processNameNode(fixture.NonContainerName, render.OutgoingInternetID). - WithCounters(map[string]int{report.Process: 1}). + AddCounter(report.Process, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], @@ -191,9 +191,7 @@ var ( WithLatests(map[string]string{ docker.ContainerHostname: fixture.ClientContainerHostname, }). - WithCounters(map[string]int{ - report.Container: 1, - }). + AddCounter(report.Container, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -206,7 +204,7 @@ var ( WithLatests(map[string]string{ docker.ContainerHostname: fixture.ServerContainerHostname, }). - WithCounters(map[string]int{report.Container: 1}). + AddCounter(report.Container, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], @@ -228,9 +226,7 @@ var ( docker.ImageID: fixture.ClientContainerImageID, docker.ImageName: fixture.ClientContainerImageName, }). - WithCounters(map[string]int{ - report.Container: 1, - }). + AddCounter(report.Container, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -240,7 +236,7 @@ var ( )), ServerContainerImageNodeID: containerImage(ServerContainerImageNodeID). - WithCounters(map[string]int{report.Container: 1}). + AddCounter(report.Container, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], @@ -258,11 +254,11 @@ var ( RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], )). - WithCounters(map[string]int{render.Pseudo: 1}) + AddCounter(render.Pseudo, 1) RenderedPods = report.Nodes{ fixture.ClientPodNodeID: pod(fixture.ClientPodNodeID, fixture.ServerPodNodeID). - WithCounters(map[string]int{report.Container: 1}). + AddCounter(report.Container, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -272,7 +268,7 @@ var ( )), fixture.ServerPodNodeID: pod(fixture.ServerPodNodeID). - WithCounters(map[string]int{report.Container: 1}). + AddCounter(report.Container, 1). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], @@ -329,7 +325,7 @@ var ( RenderedPodServices = report.Nodes{ fixture.ServiceNodeID: service(fixture.ServiceNodeID, fixture.ServiceNodeID). - WithCounters(map[string]int{report.Pod: 2}). + AddCounter(report.Pod, 2). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -353,12 +349,10 @@ var ( WithLatests(map[string]string{ report.HostName: fixture.ClientHostName, }). - WithCounters(map[string]int{ - report.Container: 1, - report.ContainerImage: 1, - report.Pod: 1, - report.Process: 2, - }). + AddCounter(report.Container, 1). + AddCounter(report.ContainerImage, 1). + AddCounter(report.Pod, 1). + AddCounter(report.Process, 2). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Client54001NodeID], RenderedEndpoints[fixture.Client54002NodeID], @@ -370,12 +364,10 @@ var ( )), fixture.ServerHostNodeID: hostNode(fixture.ServerHostNodeID, render.OutgoingInternetID). - WithCounters(map[string]int{ - report.Container: 1, - report.ContainerImage: 1, - report.Pod: 1, - report.Process: 2, - }). + AddCounter(report.Container, 1). + AddCounter(report.ContainerImage, 1). + AddCounter(report.Pod, 1). + AddCounter(report.Process, 2). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedEndpoints[fixture.NonContainerNodeID], diff --git a/render/render.go b/render/render.go index a3c3d1b49..4327277de 100644 --- a/render/render.go +++ b/render/render.go @@ -195,7 +195,7 @@ func (ret *joinResults) addUnmappedChild(m report.Node, id string, topology stri } result.Children.UnsafeAdd(m) if m.Topology != report.Endpoint { // optimisation: we never look at endpoint counts - result.Counters = result.Counters.Add(m.Topology, 1) + result = result.AddCounter(m.Topology, 1) } ret.nodes[id] = result } diff --git a/report/backcompat.go b/report/backcompat.go index 49724e27d..2848a8621 100644 --- a/report/backcompat.go +++ b/report/backcompat.go @@ -13,6 +13,7 @@ import ( type bcNode struct { Node LatestControls map[string]nodeControlDataLatestEntry `json:"latestControls,omitempty"` + Counters map[string]int `json:"counters,omitempty"` } type nodeControlDataLatestEntry struct { @@ -44,6 +45,10 @@ func (n *Node) CodecDecodeSelf(decoder *codec.Decoder) { } n.Latest = n.Latest.Set(NodeActiveControls, ts, strings.Join(cs, ScopeDelim)) } + // Counters were not generated in the Scope probe, but decode them here in case a plugin used them. + for k, v := range in.Counters { + *n = n.AddCounter(k, v) + } } type _Node Node // just so we don't recurse inside CodecEncodeSelf diff --git a/report/counters.go b/report/counters.go deleted file mode 100644 index 8bb494dcd..000000000 --- a/report/counters.go +++ /dev/null @@ -1,137 +0,0 @@ -package report - -import ( - "bytes" - "fmt" - "reflect" - - "github.com/ugorji/go/codec" - "github.com/weaveworks/ps" -) - -// Counters is a string->int map. -type Counters struct { - psMap ps.Map -} - -var emptyCounters = Counters{ps.NewMap()} - -// MakeCounters returns EmptyCounters -func MakeCounters() Counters { - return emptyCounters -} - -// Add value to the counter 'key' -func (c Counters) Add(key string, value int) Counters { - if c.psMap == nil { - c = emptyCounters - } - if existingValue, ok := c.psMap.Lookup(key); ok { - value += existingValue.(int) - } - return Counters{ - c.psMap.Set(key, value), - } -} - -// Lookup the counter 'key' -func (c Counters) Lookup(key string) (int, bool) { - if c.psMap != nil { - existingValue, ok := c.psMap.Lookup(key) - if ok { - return existingValue.(int), true - } - } - return 0, false -} - -// Size returns the number of counters -func (c Counters) Size() int { - if c.psMap == nil { - return 0 - } - return c.psMap.Size() -} - -// Merge produces a fresh Counters, container the keys from both inputs. When -// both inputs container the same key, the latter value is used. -func (c Counters) Merge(other Counters) Counters { - var ( - cSize = c.Size() - otherSize = other.Size() - output = c.psMap - iter = other.psMap - ) - switch { - case cSize == 0: - return other - case otherSize == 0: - return c - case cSize < otherSize: - output, iter = iter, output - } - iter.ForEach(func(key string, otherVal interface{}) { - if val, ok := output.Lookup(key); ok { - output = output.Set(key, otherVal.(int)+val.(int)) - } else { - output = output.Set(key, otherVal) - } - }) - return Counters{output} -} - -// String serializes Counters into a string. -func (c Counters) String() string { - buf := bytes.NewBufferString("{") - prefix := "" - for _, key := range mapKeys(c.psMap) { - val, _ := c.psMap.Lookup(key) - fmt.Fprintf(buf, "%s%s: %d", prefix, key, val.(int)) - prefix = ", " - } - fmt.Fprintf(buf, "}") - return buf.String() -} - -// DeepEqual tests equality with other Counters -func (c Counters) DeepEqual(d Counters) bool { - return mapEqual(c.psMap, d.psMap, reflect.DeepEqual) -} - -func (c Counters) fromIntermediate(in map[string]int) Counters { - out := ps.NewMap() - for k, v := range in { - out = out.Set(k, v) - } - return Counters{out} -} - -// CodecEncodeSelf implements codec.Selfer -func (c *Counters) CodecEncodeSelf(encoder *codec.Encoder) { - mapWrite(c.psMap, encoder, func(encoder *codec.Encoder, val interface{}) { - i := val.(int) - encoder.Encode(i) - }) -} - -// CodecDecodeSelf implements codec.Selfer -func (c *Counters) CodecDecodeSelf(decoder *codec.Decoder) { - out := mapRead(decoder, func(isNil bool) interface{} { - var value int - if !isNil { - decoder.Decode(&value) - } - return value - }) - *c = Counters{out} -} - -// MarshalJSON shouldn't be used, use CodecEncodeSelf instead -func (Counters) MarshalJSON() ([]byte, error) { - panic("MarshalJSON shouldn't be used, use CodecEncodeSelf instead") -} - -// UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead -func (*Counters) UnmarshalJSON(b []byte) error { - panic("UnmarshalJSON shouldn't be used, use CodecDecodeSelf instead") -} diff --git a/report/counters_internal_test.go b/report/counters_internal_test.go deleted file mode 100644 index 36fe42a1b..000000000 --- a/report/counters_internal_test.go +++ /dev/null @@ -1,152 +0,0 @@ -package report - -import ( - "bytes" - "testing" - - "github.com/ugorji/go/codec" - - "github.com/weaveworks/common/test" - "github.com/weaveworks/scope/test/reflect" -) - -func TestCountersAdd(t *testing.T) { - have := MakeCounters(). - Add("foo", 1). - Add("foo", 2) - if v, ok := have.Lookup("foo"); !ok || v != 3 { - t.Errorf("foo != 3") - } - if v, ok := have.Lookup("bar"); ok || v != 0 { - t.Errorf("bar != nil") - } -} - -func TestCountersDeepEquals(t *testing.T) { - want := MakeCounters(). - Add("foo", 3) - have := MakeCounters(). - Add("foo", 3) - if !reflect.DeepEqual(want, have) { - t.Errorf(test.Diff(want, have)) - } - notequal := MakeCounters(). - Add("foo", 4) - if reflect.DeepEqual(want, notequal) { - t.Errorf(test.Diff(want, have)) - } -} - -func TestCountersNil(t *testing.T) { - want := Counters{} - if want.Size() != 0 { - t.Errorf("nil.Size != 0") - } - if v, ok := want.Lookup("foo"); ok || v != 0 { - t.Errorf("nil.Lookup != false") - } - have := want.Add("foo", 1) - if v, ok := have.Lookup("foo"); !ok || v != 1 { - t.Errorf("nil.Add failed") - } - if have2 := want.Merge(have); !reflect.DeepEqual(have, have2) { - t.Errorf(test.Diff(have, have2)) - } -} - -func TestCountersMerge(t *testing.T) { - for name, c := range map[string]struct { - a, b, want Counters - }{ - "Empty a": { - a: MakeCounters(), - b: MakeCounters(). - Add("foo", 1), - want: MakeCounters(). - Add("foo", 1), - }, - "Empty b": { - a: MakeCounters(). - Add("foo", 1), - b: MakeCounters(), - want: MakeCounters(). - Add("foo", 1), - }, - "Disjoin a & b": { - a: MakeCounters(). - Add("foo", 1), - b: MakeCounters(). - Add("bar", 2), - want: MakeCounters(). - Add("foo", 1). - Add("bar", 2), - }, - "Overlapping a & b": { - a: MakeCounters(). - Add("foo", 1), - b: MakeCounters(). - Add("foo", 2), - want: MakeCounters(). - Add("foo", 3), - }, - } { - if have := c.a.Merge(c.b); !reflect.DeepEqual(c.want, have) { - t.Errorf("%s:\n%s", name, test.Diff(c.want, have)) - } - } -} - -func TestCountersEncoding(t *testing.T) { - want := MakeCounters(). - Add("foo", 1). - Add("bar", 2) - - { - - for _, h := range []codec.Handle{ - codec.Handle(&codec.MsgpackHandle{}), - codec.Handle(&codec.JsonHandle{}), - } { - buf := &bytes.Buffer{} - encoder := codec.NewEncoder(buf, h) - want.CodecEncodeSelf(encoder) - decoder := codec.NewDecoder(buf, h) - have := MakeCounters() - have.CodecDecodeSelf(decoder) - if !reflect.DeepEqual(want, have) { - t.Error(test.Diff(want, have)) - } - } - } - -} - -func TestCountersString(t *testing.T) { - { - var c Counters - have := c.String() - want := `{}` - if want != have { - t.Errorf("Expected: %s, Got %s", want, have) - } - } - - { - have := MakeCounters().String() - want := `{}` - if want != have { - t.Errorf("Expected: %s, Got %s", want, have) - } - } - - { - have := MakeCounters(). - Add("foo", 1). - Add("bar", 2).String() - - want := `{bar: 2, foo: 1}` - if want != have { - t.Errorf("Expected: %s, Got %s", want, have) - } - } -} diff --git a/report/map_keys.go b/report/map_keys.go index e68c8dc94..5cce1a5a7 100644 --- a/report/map_keys.go +++ b/report/map_keys.go @@ -4,6 +4,7 @@ package report const ( // Node NodeActiveControls = "active_controls" + CounterPrefix = "count_" // probe/endpoint ReverseDNSNames = "reverse_dns_names" SnoopedDNSNames = "snooped_dns_names" diff --git a/report/metadata_template.go b/report/metadata_template.go index a6a446463..40e9d6182 100644 --- a/report/metadata_template.go +++ b/report/metadata_template.go @@ -71,7 +71,7 @@ func fromSets(n Node, key string) (string, bool) { } func fromCounters(n Node, key string) (string, bool) { - val, ok := n.Counters.Lookup(key) + val, ok := n.LookupCounter(key) return strconv.Itoa(val), ok } diff --git a/report/node.go b/report/node.go index cd1398221..859398be0 100644 --- a/report/node.go +++ b/report/node.go @@ -1,6 +1,7 @@ package report import ( + "strconv" "strings" "time" @@ -13,7 +14,6 @@ import ( type Node struct { ID string `json:"id,omitempty"` Topology string `json:"topology,omitempty"` - Counters Counters `json:"counters,omitempty"` Sets Sets `json:"sets,omitempty"` Adjacency IDList `json:"adjacency,omitempty"` Latest StringLatestMap `json:"latest,omitempty"` @@ -26,7 +26,6 @@ type Node struct { func MakeNode(id string) Node { return Node{ ID: id, - Counters: MakeCounters(), Sets: MakeSets(), Adjacency: MakeIDList(), Latest: MakeStringLatestMap(), @@ -80,10 +79,25 @@ func (n Node) WithLatest(k string, ts time.Time, v string) Node { return n } -// WithCounters returns a fresh copy of n, with Counters c merged in. -func (n Node) WithCounters(c map[string]int) Node { - n.Counters = n.Counters.Merge(Counters{}.fromIntermediate(c)) - return n +// LookupCounter returns the value of a counter +// (counters are stored as strings, to keep the data structure simple) +func (n Node) LookupCounter(k string) (value int, found bool) { + name := CounterPrefix + k + var str string + if str, found = n.Latest.Lookup(name); found { + value, _ = strconv.Atoi(str) + } + return value, found +} + +// AddCounter returns a fresh copy of n, with Counter c added in. +// (counters are stored as strings, to keep the data structure simple) +func (n Node) AddCounter(k string, value int) Node { + name := CounterPrefix + k + if prevValue, found := n.LookupCounter(k); found { + value += prevValue + } + return n.WithLatest(name, mtime.Now(), strconv.Itoa(value)) } // WithSet returns a fresh copy of n, with set merged in at key. @@ -175,7 +189,6 @@ func (n Node) Merge(other Node) Node { return Node{ ID: id, Topology: topology, - Counters: n.Counters.Merge(other.Counters), Sets: n.Sets.Merge(other.Sets), Adjacency: n.Adjacency.Merge(other.Adjacency), Latest: n.Latest.Merge(other.Latest), diff --git a/report/node_test.go b/report/node_test.go index 5751fe922..9706be327 100644 --- a/report/node_test.go +++ b/report/node_test.go @@ -146,33 +146,37 @@ func TestMergeNodes(t *testing.T) { }), }, }, - "Counters": { - a: report.Nodes{ - "1": report.MakeNode("1").WithCounters(map[string]int{ - "a": 13, - "b": 57, - "c": 89, - }), - }, - b: report.Nodes{ - "1": report.MakeNode("1").WithCounters(map[string]int{ - "a": 78, - "b": 3, - "d": 47, - }), - }, - want: report.Nodes{ - "1": report.MakeNode("1").WithCounters(map[string]int{ - "a": 91, - "b": 60, - "c": 89, - "d": 47, - }), - }, - }, + // Note we previously tested that counters merged by adding, + // but that was a bug: merges must be idempotent. + // Counters merge like other 'latest' values now. } { if have := c.a.Merge(c.b); !reflect.DeepEqual(c.want, have) { t.Errorf("%s: %s", name, test.Diff(c.want, have)) } } } + +func TestCounters(t *testing.T) { + mtime.NowForce(time.Now()) + defer mtime.NowReset() + + a := report.MakeNode("1"). + AddCounter("a", 13). + AddCounter("b", 57). + AddCounter("c", 89) + + b := a. + AddCounter("a", 78). + AddCounter("b", 3). + AddCounter("d", 47) + + want := report.MakeNode("1"). + AddCounter("a", 91). + AddCounter("b", 60). + AddCounter("c", 89). + AddCounter("d", 47) + + if have := b; !reflect.DeepEqual(want, have) { + t.Errorf("Counters: %s", test.Diff(want, have)) + } +} diff --git a/report/report_test.go b/report/report_test.go index 80717e7f6..753408527 100644 --- a/report/report_test.go +++ b/report/report_test.go @@ -57,10 +57,8 @@ func TestNode(t *testing.T) { } } { - node := report.MakeNode("foo").WithCounters( - map[string]int{"foo": 1}, - ) - if value, _ := node.Counters.Lookup("foo"); value != 1 { + node := report.MakeNode("foo").AddCounter("foo", 1) + if value, _ := node.LookupCounter("foo"); value != 1 { t.Errorf("want foo, have %d", value) } } diff --git a/test/utils/prune.go b/test/utils/prune.go index 0e1affc16..69860e913 100644 --- a/test/utils/prune.go +++ b/test/utils/prune.go @@ -1,6 +1,9 @@ package utils import ( + "strings" + "time" + "github.com/weaveworks/scope/report" ) @@ -27,6 +30,11 @@ func PruneNode(node report.Node) report.Node { WithTopology(node.Topology). WithAdjacent(node.Adjacency...). WithChildren(prunedChildren) - prunedNode.Counters = node.Counters + // Copy counters across, but with zero timestamp so they compare equal + node.Latest.ForEach(func(k string, _ time.Time, v string) { + if strings.HasPrefix(k, report.CounterPrefix) { + prunedNode.Latest = prunedNode.Latest.Set(k, time.Time{}, v) + } + }) return prunedNode } From 62d4fbd94e9c34531043eb98fe63ffd58c800c89 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 10 Mar 2020 11:10:35 +0000 Subject: [PATCH 3/4] Add a second container to fixture to check counts --- render/expected/expected.go | 13 ++++++++++--- test/fixture/report_fixture.go | 30 ++++++++++++++++++++++++------ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/render/expected/expected.go b/render/expected/expected.go index b11137f2f..7ce945e74 100644 --- a/render/expected/expected.go +++ b/render/expected/expected.go @@ -181,6 +181,8 @@ var ( RenderedProcesses[fixture.ServerProcessNodeID], )), + fixture.ServerContainer2NodeID: container(fixture.ServerContainer2NodeID), + uncontainedServerID: uncontainedServerNode, render.IncomingInternetID: theIncomingInternetNode(fixture.ServerContainerNodeID), render.OutgoingInternetID: theOutgoingInternetNode, @@ -204,11 +206,12 @@ var ( WithLatests(map[string]string{ docker.ContainerHostname: fixture.ServerContainerHostname, }). - AddCounter(report.Container, 1). + AddCounter(report.Container, 2). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], RenderedContainers[fixture.ServerContainerNodeID], + RenderedContainers[fixture.ServerContainer2NodeID], )), uncontainedServerID: uncontainedServerNode, @@ -236,11 +239,12 @@ var ( )), ServerContainerImageNodeID: containerImage(ServerContainerImageNodeID). - AddCounter(report.Container, 1). + AddCounter(report.Container, 2). WithChildren(report.MakeNodeSet( RenderedEndpoints[fixture.Server80NodeID], RenderedProcesses[fixture.ServerProcessNodeID], RenderedContainers[fixture.ServerContainerNodeID], + RenderedContainers[fixture.ServerContainer2NodeID], )), uncontainedServerID: uncontainedServerNode, @@ -253,7 +257,9 @@ var ( uncontainedServerNode, RenderedEndpoints[fixture.NonContainerNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], + RenderedContainers[fixture.ServerContainer2NodeID], )). + AddCounter(report.Container, 1). AddCounter(render.Pseudo, 1) RenderedPods = report.Nodes{ @@ -364,7 +370,7 @@ var ( )), fixture.ServerHostNodeID: hostNode(fixture.ServerHostNodeID, render.OutgoingInternetID). - AddCounter(report.Container, 1). + AddCounter(report.Container, 2). AddCounter(report.ContainerImage, 1). AddCounter(report.Pod, 1). AddCounter(report.Process, 2). @@ -374,6 +380,7 @@ var ( RenderedProcesses[fixture.ServerProcessNodeID], RenderedProcesses[fixture.NonContainerProcessNodeID], RenderedContainers[fixture.ServerContainerNodeID], + RenderedContainers[fixture.ServerContainer2NodeID], RenderedContainerImages[ServerContainerImageNodeID], RenderedPods[fixture.ServerPodNodeID], )), diff --git a/test/fixture/report_fixture.go b/test/fixture/report_fixture.go index 532e13aa3..8a5d2ba56 100644 --- a/test/fixture/report_fixture.go +++ b/test/fixture/report_fixture.go @@ -71,12 +71,15 @@ var ( ServerProcessNodeID = report.MakeProcessNodeID(ServerHostID, ServerPID) NonContainerProcessNodeID = report.MakeProcessNodeID(ServerHostID, NonContainerPID) - ClientContainerID = "a1b2c3d4e5" - ClientContainerName = "client" - ServerContainerID = "5e4d3c2b1a" - ServerContainerName = "task-name-5-server-aceb93e2f2b797caba01" - ClientContainerNodeID = report.MakeContainerNodeID(ClientContainerID) - ServerContainerNodeID = report.MakeContainerNodeID(ServerContainerID) + ClientContainerID = "a1b2c3d4e5" + ClientContainerName = "client" + ServerContainerID = "5e4d3c2b1a" + ServerContainerName = "task-name-5-server-aceb93e2f2b797caba01" + ServerContainer2ID = "1a1d30201f" + ServerContainer2Name = "task-name-6-server-8213182737" + ClientContainerNodeID = report.MakeContainerNodeID(ClientContainerID) + ServerContainerNodeID = report.MakeContainerNodeID(ServerContainerID) + ServerContainer2NodeID = report.MakeContainerNodeID(ServerContainer2ID) TestLabelKey1 = "myrole" ApplicationLabelValue1 = "customapplication1" @@ -260,6 +263,21 @@ var ( docker.CPUTotalUsage: ServerContainerCPUMetric, docker.MemoryUsage: ServerContainerMemoryMetric, }), + + // Two server containers with same hostname + ServerContainer2NodeID: report.MakeNodeWith( + ServerContainer2NodeID, map[string]string{ + docker.ContainerID: ServerContainer2ID, + docker.ContainerName: ServerContainer2Name, + docker.ContainerHostname: ServerContainerHostname, + docker.ContainerState: report.StateRunning, + docker.ContainerStateHuman: report.StateRunning, + docker.ImageID: ServerContainerImageID, + report.HostNodeID: ServerHostNodeID, + }). + WithTopology(report.Container).WithParents(report.MakeSets(). + Add("host", report.MakeStringSet(ServerHostNodeID)). + Add("container_image", report.MakeStringSet(ServerContainerImageNodeID))), }, MetadataTemplates: docker.ContainerMetadataTemplates, MetricTemplates: docker.ContainerMetricTemplates, From ffa90719b0504531aa25b18de12d64cfdff51ac1 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 10 Mar 2020 09:58:21 +0000 Subject: [PATCH 4/4] Change containers-by-hostname renderer from Map to Renderer It was relying on the previous behaviour of Merge() adding up counters. --- render/container.go | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/render/container.go b/render/container.go index da3042c84..347d56745 100644 --- a/render/container.go +++ b/render/container.go @@ -169,10 +169,7 @@ var ContainerImageRenderer = Memoise(FilterEmpty(report.Container, // // not memoised var ContainerHostnameRenderer = FilterEmpty(report.Container, - MakeMap( - MapContainer2Hostname, - ContainerWithImageNameRenderer, - ), + containerHostnameRenderer{}, ) var portMappingMatch = regexp.MustCompile(`([0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}):([0-9]+)->([0-9]+)/tcp`) @@ -312,21 +309,24 @@ func MapContainerImage2Name(n report.Node) report.Node { var containerHostnameTopology = MakeGroupNodeTopology(report.Container, report.DockerContainerHostname) -// MapContainer2Hostname maps container Nodes to 'hostname' renderabled nodes.. -func MapContainer2Hostname(n report.Node) report.Node { - // Propagate all pseudo nodes - if n.Topology == Pseudo { - return n - } +// containerHostnameRenderer collects containers by docker hostname +type containerHostnameRenderer struct{} - // Otherwise, if some some reason the container doesn't have a hostname - // (maybe slightly out of sync reports), just drop it - id, ok := n.Latest.Lookup(report.DockerContainerHostname) - if !ok { - return report.Node{} - } +func (m containerHostnameRenderer) Render(ctx context.Context, rpt report.Report) Nodes { + containers := ContainerWithImageNameRenderer.Render(ctx, rpt) + ret := newJoinResults(nil) - node := NewDerivedNode(id, n).WithTopology(containerHostnameTopology). - AddCounter(n.Topology, 1) - return node + for _, n := range containers.Nodes { + if n.Topology == Pseudo { + ret.passThrough(n) + continue + } + // If some some reason the container doesn't have a hostname, just drop it + id, ok := n.Latest.Lookup(report.DockerContainerHostname) + if !ok { + continue + } + ret.addChildAndChildren(n, id, containerHostnameTopology) + } + return ret.result(containers) }