From 651e42e54ec89375c6ed9bd17ab4c3815f197aeb Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 23 Dec 2017 22:21:24 +0000 Subject: [PATCH 1/3] fix accidental report fixture modification Report.Copy() shallow-copies the nodes in Report.Nodes. Hence Node.Metrics is shared between the original and the copy. Hence bad things happen when modifying it. This bug has laid dormant because by luck other tests in detailed_test involving metrics are executed first. --- render/detailed/summary_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/render/detailed/summary_test.go b/render/detailed/summary_test.go index 9d142b4a2..fcc375586 100644 --- a/render/detailed/summary_test.go +++ b/render/detailed/summary_test.go @@ -49,8 +49,10 @@ func TestSummaries(t *testing.T) { t1, t2 := mtime.Now().Add(-1*time.Minute), mtime.Now() metric := report.MakeMetric([]report.Sample{{Timestamp: t1, Value: 1}, {Timestamp: t2, Value: 2}}) input := fixture.Report.Copy() - - input.Process.Nodes[fixture.ClientProcess1NodeID].Metrics[process.CPUUsage] = metric + processNode := input.Process.Nodes[fixture.ClientProcess1NodeID] + processNode.Metrics = processNode.Metrics.Copy() + processNode.Metrics[process.CPUUsage] = metric + input.Process.Nodes[fixture.ClientProcess1NodeID] = processNode have := detailed.Summaries(report.RenderContext{Report: input}, render.ProcessRenderer.Render(input).Nodes) node, ok := have[fixture.ClientProcess1NodeID] From d861b41837fa6ec5e9aa87c6e129023523ab6312 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 23 Dec 2017 22:34:45 +0000 Subject: [PATCH 2/3] refactor: inline summarisation of metadata, metrics, tables This removes a bunch of duplication and scattering of little pieces of code. --- render/detailed/metadata.go | 19 --- render/detailed/metadata_test.go | 49 ------- render/detailed/metrics.go | 20 --- render/detailed/metrics_test.go | 145 ------------------- render/detailed/summary.go | 15 +- render/detailed/summary_test.go | 236 +++++++++++++++++++++++++++++++ render/detailed/tables.go | 20 --- render/detailed/tables_test.go | 78 ---------- 8 files changed, 247 insertions(+), 335 deletions(-) delete mode 100644 render/detailed/metadata.go delete mode 100644 render/detailed/metadata_test.go delete mode 100644 render/detailed/metrics.go delete mode 100644 render/detailed/metrics_test.go delete mode 100644 render/detailed/tables.go delete mode 100644 render/detailed/tables_test.go diff --git a/render/detailed/metadata.go b/render/detailed/metadata.go deleted file mode 100644 index c6a3d71a0..000000000 --- a/render/detailed/metadata.go +++ /dev/null @@ -1,19 +0,0 @@ -package detailed - -import ( - "github.com/weaveworks/scope/report" -) - -// NodeMetadata produces a table (to be consumed directly by the UI) based on -// an a report.Node, which is (hopefully) a node in one of our topologies. -func NodeMetadata(r report.Report, n report.Node) []report.MetadataRow { - if _, ok := n.Counters.Lookup(n.Topology); ok { - // This is a group of nodes, so no metadata! - return nil - } - - if topology, ok := r.Topology(n.Topology); ok { - return topology.MetadataTemplates.MetadataRows(n) - } - return nil -} diff --git a/render/detailed/metadata_test.go b/render/detailed/metadata_test.go deleted file mode 100644 index 180e1c39c..000000000 --- a/render/detailed/metadata_test.go +++ /dev/null @@ -1,49 +0,0 @@ -package detailed_test - -import ( - "reflect" - "testing" - - "github.com/weaveworks/common/test" - "github.com/weaveworks/scope/probe/docker" - "github.com/weaveworks/scope/render/detailed" - "github.com/weaveworks/scope/report" - "github.com/weaveworks/scope/test/fixture" -) - -func TestNodeMetadata(t *testing.T) { - inputs := []struct { - name string - node report.Node - want []report.MetadataRow - }{ - { - name: "container", - node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ - docker.ContainerID: fixture.ClientContainerID, - docker.LabelPrefix + "label1": "label1value", - docker.ContainerStateHuman: docker.StateRunning, - }).WithTopology(report.Container).WithSets(report.MakeSets(). - Add(docker.ContainerIPs, report.MakeStringSet("10.10.10.0/24", "10.10.10.1/24")), - ), - want: []report.MetadataRow{ - {ID: docker.ContainerStateHuman, Label: "State", Value: "running", Priority: 3}, - {ID: docker.ContainerIPs, Label: "IPs", Value: "10.10.10.0/24, 10.10.10.1/24", Priority: 7}, - {ID: docker.ContainerID, Label: "ID", Value: fixture.ClientContainerID, Priority: 10, Truncate: 12}, - }, - }, - { - name: "unknown topology", - node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ - docker.ContainerID: fixture.ClientContainerID, - }).WithTopology("foobar"), - want: nil, - }, - } - for _, input := range inputs { - have := detailed.NodeMetadata(fixture.Report, input.node) - if !reflect.DeepEqual(input.want, have) { - t.Errorf("%s: %s", input.name, test.Diff(input.want, have)) - } - } -} diff --git a/render/detailed/metrics.go b/render/detailed/metrics.go deleted file mode 100644 index ad5935e59..000000000 --- a/render/detailed/metrics.go +++ /dev/null @@ -1,20 +0,0 @@ -package detailed - -import ( - "github.com/weaveworks/scope/report" -) - -// NodeMetrics produces a table (to be consumed directly by the UI) based on -// an a report.Node, which is (hopefully) a node in one of our topologies. -func NodeMetrics(r report.Report, n report.Node) []report.MetricRow { - if _, ok := n.Counters.Lookup(n.Topology); ok { - // This is a group of nodes, so no metrics! - return nil - } - - topology, ok := r.Topology(n.Topology) - if !ok { - return nil - } - return topology.MetricTemplates.MetricRows(n) -} diff --git a/render/detailed/metrics_test.go b/render/detailed/metrics_test.go deleted file mode 100644 index 6444afc17..000000000 --- a/render/detailed/metrics_test.go +++ /dev/null @@ -1,145 +0,0 @@ -package detailed_test - -import ( - "reflect" - "testing" - "time" - - "github.com/weaveworks/common/test" - "github.com/weaveworks/scope/probe/docker" - "github.com/weaveworks/scope/probe/host" - "github.com/weaveworks/scope/probe/process" - "github.com/weaveworks/scope/render/detailed" - "github.com/weaveworks/scope/report" - "github.com/weaveworks/scope/test/fixture" -) - -func TestNodeMetrics(t *testing.T) { - inputs := []struct { - name string - node report.Node - want []report.MetricRow - }{ - { - name: "process", - node: fixture.Report.Process.Nodes[fixture.ClientProcess1NodeID], - want: []report.MetricRow{ - { - ID: process.CPUUsage, - Label: "CPU", - Format: "percent", - Group: "", - Value: 0.01, - Priority: 1, - Metric: &fixture.ClientProcess1CPUMetric, - }, - { - ID: process.MemoryUsage, - Label: "Memory", - Format: "filesize", - Group: "", - Value: 0.02, - Priority: 2, - Metric: &fixture.ClientProcess1MemoryMetric, - }, - }, - }, - { - name: "container", - node: fixture.Report.Container.Nodes[fixture.ClientContainerNodeID], - want: []report.MetricRow{ - { - ID: docker.CPUTotalUsage, - Label: "CPU", - Format: "percent", - Group: "", - Value: 0.03, - Priority: 1, - Metric: &fixture.ClientContainerCPUMetric, - }, - { - ID: docker.MemoryUsage, - Label: "Memory", - Format: "filesize", - Group: "", - Value: 0.04, - Priority: 2, - Metric: &fixture.ClientContainerMemoryMetric, - }, - }, - }, - { - name: "host", - node: fixture.Report.Host.Nodes[fixture.ClientHostNodeID], - want: []report.MetricRow{ - { - ID: host.CPUUsage, - Label: "CPU", - Format: "percent", - Group: "", - Value: 0.07, - Priority: 1, - Metric: &fixture.ClientHostCPUMetric, - }, - { - ID: host.MemoryUsage, - Label: "Memory", - Format: "filesize", - Group: "", - Value: 0.08, - Priority: 2, - Metric: &fixture.ClientHostMemoryMetric, - }, - { - ID: host.Load1, - Label: "Load (1m)", - Group: "load", - Value: 0.09, - Priority: 11, - Metric: &fixture.ClientHostLoad1Metric, - }, - }, - }, - { - name: "unknown topology", - node: report.MakeNode(fixture.ClientContainerNodeID).WithTopology("foobar"), - want: nil, - }, - } - for _, input := range inputs { - have := detailed.NodeMetrics(fixture.Report, input.node) - if !reflect.DeepEqual(input.want, have) { - t.Errorf("%s: %s", input.name, test.Diff(input.want, have)) - } - } -} - -func TestMetricRowSummary(t *testing.T) { - var ( - now = time.Now() - metric = report.MakeSingletonMetric(now, 1.234) - row = report.MetricRow{ - ID: "id", - Format: "format", - Group: "group", - Value: 1.234, - Priority: 1, - Metric: &metric, - } - summary = row.Summary() - ) - // summary should not have any samples - if summary.Metric.Len() != 0 { - t.Errorf("Expected summary to have no samples, but had %d", summary.Metric.Len()) - } - // original metric should still have its samples - if metric.Len() != 1 { - t.Errorf("Expected original metric to still have it's samples, but had %d", metric.Len()) - } - // summary should have all the same fields (minus the metric) - summary.Metric = nil - row.Metric = nil - if !reflect.DeepEqual(summary, row) { - t.Errorf("Expected summary to have same fields as original: %s", test.Diff(summary, row)) - } -} diff --git a/render/detailed/summary.go b/render/detailed/summary.go index 9cb56fbd7..0d0b24af0 100644 --- a/render/detailed/summary.go +++ b/render/detailed/summary.go @@ -135,16 +135,23 @@ func (n NodeSummary) SummarizeMetrics() NodeSummary { func baseNodeSummary(r report.Report, n report.Node) NodeSummary { t, _ := r.Topology(n.Topology) - return NodeSummary{ + summary := NodeSummary{ ID: n.ID, Shape: t.GetShape(), Linkable: true, - Metadata: NodeMetadata(r, n), - Metrics: NodeMetrics(r, n), Parents: Parents(r, n), - Tables: NodeTables(r, n), Adjacency: n.Adjacency, } + if _, ok := n.Counters.Lookup(n.Topology); ok { + // This is a group of nodes, so no metadata, metrics, tables + return summary + } + if topology, ok := r.Topology(n.Topology); ok { + summary.Metadata = topology.MetadataTemplates.MetadataRows(n) + summary.Metrics = topology.MetricTemplates.MetricRows(n) + summary.Tables = topology.TableTemplates.Tables(n) + } + return summary } func pseudoNodeSummary(base NodeSummary, n report.Node) (NodeSummary, bool) { diff --git a/render/detailed/summary_test.go b/render/detailed/summary_test.go index fcc375586..b110ea2f8 100644 --- a/render/detailed/summary_test.go +++ b/render/detailed/summary_test.go @@ -197,3 +197,239 @@ func TestMakeNodeSummary(t *testing.T) { } } } + +func TestNodeMetadata(t *testing.T) { + inputs := []struct { + name string + node report.Node + want []report.MetadataRow + }{ + { + name: "container", + node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ + docker.ContainerID: fixture.ClientContainerID, + docker.LabelPrefix + "label1": "label1value", + docker.ContainerStateHuman: docker.StateRunning, + }).WithTopology(report.Container).WithSets(report.MakeSets(). + Add(docker.ContainerIPs, report.MakeStringSet("10.10.10.0/24", "10.10.10.1/24")), + ), + want: []report.MetadataRow{ + {ID: docker.ContainerStateHuman, Label: "State", Value: "running", Priority: 3}, + {ID: docker.ContainerIPs, Label: "IPs", Value: "10.10.10.0/24, 10.10.10.1/24", Priority: 7}, + {ID: docker.ContainerID, Label: "ID", Value: fixture.ClientContainerID, Priority: 10, Truncate: 12}, + }, + }, + { + name: "unknown topology", + node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ + docker.ContainerID: fixture.ClientContainerID, + }).WithTopology("foobar"), + want: nil, + }, + } + for _, input := range inputs { + summary, _ := detailed.MakeNodeSummary(report.RenderContext{Report: fixture.Report}, input.node) + have := summary.Metadata + if !reflect.DeepEqual(input.want, have) { + t.Errorf("%s: %s", input.name, test.Diff(input.want, have)) + } + } +} + +func TestNodeMetrics(t *testing.T) { + inputs := []struct { + name string + node report.Node + want []report.MetricRow + }{ + { + name: "process", + node: fixture.Report.Process.Nodes[fixture.ClientProcess1NodeID], + want: []report.MetricRow{ + { + ID: process.CPUUsage, + Label: "CPU", + Format: "percent", + Group: "", + Value: 0.01, + Priority: 1, + Metric: &fixture.ClientProcess1CPUMetric, + }, + { + ID: process.MemoryUsage, + Label: "Memory", + Format: "filesize", + Group: "", + Value: 0.02, + Priority: 2, + Metric: &fixture.ClientProcess1MemoryMetric, + }, + }, + }, + { + name: "container", + node: fixture.Report.Container.Nodes[fixture.ClientContainerNodeID], + want: []report.MetricRow{ + { + ID: docker.CPUTotalUsage, + Label: "CPU", + Format: "percent", + Group: "", + Value: 0.03, + Priority: 1, + Metric: &fixture.ClientContainerCPUMetric, + }, + { + ID: docker.MemoryUsage, + Label: "Memory", + Format: "filesize", + Group: "", + Value: 0.04, + Priority: 2, + Metric: &fixture.ClientContainerMemoryMetric, + }, + }, + }, + { + name: "host", + node: fixture.Report.Host.Nodes[fixture.ClientHostNodeID], + want: []report.MetricRow{ + { + ID: host.CPUUsage, + Label: "CPU", + Format: "percent", + Group: "", + Value: 0.07, + Priority: 1, + Metric: &fixture.ClientHostCPUMetric, + }, + { + ID: host.MemoryUsage, + Label: "Memory", + Format: "filesize", + Group: "", + Value: 0.08, + Priority: 2, + Metric: &fixture.ClientHostMemoryMetric, + }, + { + ID: host.Load1, + Label: "Load (1m)", + Group: "load", + Value: 0.09, + Priority: 11, + Metric: &fixture.ClientHostLoad1Metric, + }, + }, + }, + { + name: "unknown topology", + node: report.MakeNode(fixture.ClientContainerNodeID).WithTopology("foobar"), + want: nil, + }, + } + for _, input := range inputs { + summary, _ := detailed.MakeNodeSummary(report.RenderContext{Report: fixture.Report}, input.node) + have := summary.Metrics + if !reflect.DeepEqual(input.want, have) { + t.Errorf("%s: %s", input.name, test.Diff(input.want, have)) + } + } +} + +func TestMetricRowSummary(t *testing.T) { + var ( + now = time.Now() + metric = report.MakeSingletonMetric(now, 1.234) + row = report.MetricRow{ + ID: "id", + Format: "format", + Group: "group", + Value: 1.234, + Priority: 1, + Metric: &metric, + } + summary = row.Summary() + ) + // summary should not have any samples + if summary.Metric.Len() != 0 { + t.Errorf("Expected summary to have no samples, but had %d", summary.Metric.Len()) + } + // original metric should still have its samples + if metric.Len() != 1 { + t.Errorf("Expected original metric to still have it's samples, but had %d", metric.Len()) + } + // summary should have all the same fields (minus the metric) + summary.Metric = nil + row.Metric = nil + if !reflect.DeepEqual(summary, row) { + t.Errorf("Expected summary to have same fields as original: %s", test.Diff(summary, row)) + } +} + +func TestNodeTables(t *testing.T) { + inputs := []struct { + name string + rpt report.Report + node report.Node + want []report.Table + }{ + { + name: "container", + rpt: report.Report{ + Container: report.MakeTopology(). + WithTableTemplates(docker.ContainerTableTemplates), + }, + node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ + docker.ContainerID: fixture.ClientContainerID, + docker.LabelPrefix + "label1": "label1value", + docker.ContainerState: docker.StateRunning, + }).WithTopology(report.Container).WithSets(report.MakeSets(). + Add(docker.ContainerIPs, report.MakeStringSet("10.10.10.0/24", "10.10.10.1/24")), + ), + want: []report.Table{ + { + ID: docker.EnvPrefix, + Type: report.PropertyListType, + Label: "Environment Variables", + Rows: []report.Row{}, + }, + { + ID: docker.LabelPrefix, + Type: report.PropertyListType, + Label: "Docker Labels", + Rows: []report.Row{ + { + ID: "label_label1", + Entries: map[string]string{ + "label": "label1", + "value": "label1value", + }, + }, + }, + }, + { + ID: docker.ImageTableID, + Type: report.PropertyListType, + Label: "Image", + Rows: []report.Row{}, + }, + }, + }, + { + name: "unknown topology", + rpt: report.MakeReport(), + node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ + docker.ContainerID: fixture.ClientContainerID, + }).WithTopology("foobar"), + want: nil, + }, + } + for _, input := range inputs { + summary, _ := detailed.MakeNodeSummary(report.RenderContext{Report: input.rpt}, input.node) + have := summary.Tables + if !reflect.DeepEqual(input.want, have) { + t.Errorf("%s: %s", input.name, test.Diff(input.want, have)) + } + } +} diff --git a/render/detailed/tables.go b/render/detailed/tables.go deleted file mode 100644 index 63fe91878..000000000 --- a/render/detailed/tables.go +++ /dev/null @@ -1,20 +0,0 @@ -package detailed - -import ( - "github.com/weaveworks/scope/report" -) - -// NodeTables produces a list of tables (to be consumed directly by the UI) based -// on the report and the node. It uses the report to get the templates for the node's -// topology. -func NodeTables(r report.Report, n report.Node) []report.Table { - if _, ok := n.Counters.Lookup(n.Topology); ok { - // This is a group of nodes, so no tables! - return nil - } - - if topology, ok := r.Topology(n.Topology); ok { - return topology.TableTemplates.Tables(n) - } - return nil -} diff --git a/render/detailed/tables_test.go b/render/detailed/tables_test.go deleted file mode 100644 index 1ae83afd4..000000000 --- a/render/detailed/tables_test.go +++ /dev/null @@ -1,78 +0,0 @@ -package detailed_test - -import ( - "reflect" - "testing" - - "github.com/weaveworks/common/test" - "github.com/weaveworks/scope/probe/docker" - "github.com/weaveworks/scope/render/detailed" - "github.com/weaveworks/scope/report" - "github.com/weaveworks/scope/test/fixture" -) - -func TestNodeTables(t *testing.T) { - inputs := []struct { - name string - rpt report.Report - node report.Node - want []report.Table - }{ - { - name: "container", - rpt: report.Report{ - Container: report.MakeTopology(). - WithTableTemplates(docker.ContainerTableTemplates), - }, - node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ - docker.ContainerID: fixture.ClientContainerID, - docker.LabelPrefix + "label1": "label1value", - docker.ContainerState: docker.StateRunning, - }).WithTopology(report.Container).WithSets(report.MakeSets(). - Add(docker.ContainerIPs, report.MakeStringSet("10.10.10.0/24", "10.10.10.1/24")), - ), - want: []report.Table{ - { - ID: docker.EnvPrefix, - Type: report.PropertyListType, - Label: "Environment Variables", - Rows: []report.Row{}, - }, - { - ID: docker.LabelPrefix, - Type: report.PropertyListType, - Label: "Docker Labels", - Rows: []report.Row{ - { - ID: "label_label1", - Entries: map[string]string{ - "label": "label1", - "value": "label1value", - }, - }, - }, - }, - { - ID: docker.ImageTableID, - Type: report.PropertyListType, - Label: "Image", - Rows: []report.Row{}, - }, - }, - }, - { - name: "unknown topology", - rpt: report.MakeReport(), - node: report.MakeNodeWith(fixture.ClientContainerNodeID, map[string]string{ - docker.ContainerID: fixture.ClientContainerID, - }).WithTopology("foobar"), - want: nil, - }, - } - for _, input := range inputs { - have := detailed.NodeTables(input.rpt, input.node) - if !reflect.DeepEqual(input.want, have) { - t.Errorf("%s: %s", input.name, test.Diff(input.want, have)) - } - } -} From fc66827af7d7e77d4ec0b85503dd243a62378d0d Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 23 Dec 2017 22:39:08 +0000 Subject: [PATCH 3/3] refactor: don't set shape based on unitialised topology This doesn't make any difference to the outcome - we were simply setting the shape in the NodeSummary to "", which is what it starts out as - but looks less weird in the code. --- render/detailed/summary.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/render/detailed/summary.go b/render/detailed/summary.go index 0d0b24af0..507b586df 100644 --- a/render/detailed/summary.go +++ b/render/detailed/summary.go @@ -134,14 +134,15 @@ func (n NodeSummary) SummarizeMetrics() NodeSummary { } func baseNodeSummary(r report.Report, n report.Node) NodeSummary { - t, _ := r.Topology(n.Topology) summary := NodeSummary{ ID: n.ID, - Shape: t.GetShape(), Linkable: true, Parents: Parents(r, n), Adjacency: n.Adjacency, } + if t, ok := r.Topology(n.Topology); ok { + summary.Shape = t.GetShape() + } if _, ok := n.Counters.Lookup(n.Topology); ok { // This is a group of nodes, so no metadata, metrics, tables return summary