Review feedback

app/api_topologies.go

@@ -3,6 +3,7 @@ package app
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 	"sort"
 	"sync"
 
@@ -269,15 +270,11 @@ func (r *registry) makeTopologyList(rep Reporter) CtxHandlerFunc {
 func (r *registry) renderTopologies(rpt report.Report, req *http.Request) []APITopologyDesc {
 	topologies := []APITopologyDesc{}
 	req.ParseForm()
-	values := map[string]string{}
-	for k, vs := range req.Form {
-		values[k] = vs[0]
-	}
 	r.walk(func(desc APITopologyDesc) {
-		renderer, decorator, _ := r.rendererForTopology(desc.id, values, rpt)
+		renderer, decorator, _ := r.rendererForTopology(desc.id, req.Form, rpt)
 		desc.Stats = decorateWithStats(rpt, renderer, decorator)
 		for i, sub := range desc.SubTopologies {
-			renderer, decorator, _ := r.rendererForTopology(sub.id, values, rpt)
+			renderer, decorator, _ := r.rendererForTopology(sub.id, req.Form, rpt)
 			desc.SubTopologies[i].Stats = decorateWithStats(rpt, renderer, decorator)
 		}
 		topologies = append(topologies, desc)
@@ -307,16 +304,16 @@ func decorateWithStats(rpt report.Report, renderer render.Renderer, decorator re
 	}
 }
 
-func (r *registry) rendererForTopology(id string, values map[string]string, rpt report.Report) (render.Renderer, render.Decorator, error) {
-	topology, ok := r.get(id)
+func (r *registry) rendererForTopology(topologyID string, values url.Values, rpt report.Report) (render.Renderer, render.Decorator, error) {
+	topology, ok := r.get(topologyID)
 	if !ok {
-		return nil, nil, fmt.Errorf("topology not found: %s", id)
+		return nil, nil, fmt.Errorf("topology not found: %s", topologyID)
 	}
 	topology = updateFilters(rpt, []APITopologyDesc{topology})[0]
 
 	var filters []render.FilterFunc
 	for _, group := range topology.Options {
-		value := values[group.ID]
+		value := values.Get(group.ID)
 		for _, opt := range group.Options {
 			if opt.filter == nil {
 				continue
@@ -335,19 +332,34 @@ func (r *registry) rendererForTopology(id string, values map[string]string, rpt
 	return topology.renderer, decorator, nil
 }
 
-type reportRenderHandler func(context.Context, Reporter, http.ResponseWriter, *http.Request)
+type reporterHandler func(context.Context, Reporter, http.ResponseWriter, *http.Request)
 
-func (r *registry) rendererForRequest(req *http.Request, rpt report.Report) (render.Renderer, render.Decorator, error) {
-	req.ParseForm()
-	values := map[string]string{}
-	for k, vs := range req.Form {
-		values[k] = vs[0]
+func captureReporter(rep Reporter, f reporterHandler) CtxHandlerFunc {
+	return func(ctx context.Context, w http.ResponseWriter, r *http.Request) {
+		f(ctx, rep, w, r)
 	}
-	return r.rendererForTopology(mux.Vars(req)["topology"], values, rpt)
 }
 
-func captureReporter(rep Reporter, f reportRenderHandler) CtxHandlerFunc {
-	return func(ctx context.Context, w http.ResponseWriter, r *http.Request) {
-		f(ctx, rep, w, r)
+type rendererHandler func(context.Context, render.Renderer, render.Decorator, report.Report, http.ResponseWriter, *http.Request)
+
+func (r *registry) captureRenderer(rep Reporter, f rendererHandler) CtxHandlerFunc {
+	return func(ctx context.Context, w http.ResponseWriter, req *http.Request) {
+		topologyID := mux.Vars(req)["topology"]
+		if _, ok := r.get(topologyID); !ok {
+			http.NotFound(w, req)
+			return
+		}
+		rpt, err := rep.Report(ctx)
+		if err != nil {
+			respondWith(w, http.StatusInternalServerError, err.Error())
+			return
+		}
+		req.ParseForm()
+		renderer, decorator, err := r.rendererForTopology(topologyID, req.Form, rpt)
+		if err != nil {
+			respondWith(w, http.StatusInternalServerError, err.Error())
+			return
+		}
+		f(ctx, renderer, decorator, rpt, w, req)
 	}
 }

app/api_topology.go

@@ -9,7 +9,9 @@ import (
 	"golang.org/x/net/context"
 
 	"github.com/weaveworks/scope/common/xfer"
+	"github.com/weaveworks/scope/render"
 	"github.com/weaveworks/scope/render/detailed"
+	"github.com/weaveworks/scope/report"
 )
 
 const (
@@ -27,80 +29,48 @@ type APINode struct {
 }
 
 // Full topology.
-func handleTopology(ctx context.Context, rep Reporter, w http.ResponseWriter, r *http.Request) {
-	report, err := rep.Report(ctx)
-	if err != nil {
-		respondWith(w, http.StatusInternalServerError, err.Error())
-		return
-	}
-	renderer, decorator, err := topologyRegistry.rendererForRequest(r, report)
-	if err != nil {
-		http.NotFound(w, r)
-		return
-	}
+func handleTopology(ctx context.Context, renderer render.Renderer, decorator render.Decorator, report report.Report, w http.ResponseWriter, r *http.Request) {
 	respondWith(w, http.StatusOK, APITopology{
 		Nodes: detailed.Summaries(report, renderer.Render(report, decorator)),
 	})
 }
 
-// Websocket for the full topology. This route overlaps with the next.
-func handleWs(ctx context.Context, rep Reporter, w http.ResponseWriter, r *http.Request) {
-	if err := r.ParseForm(); err != nil {
-		respondWith(w, http.StatusInternalServerError, err.Error())
-		return
-	}
-	loop := websocketLoop
-	if t := r.Form.Get("t"); t != "" {
-		var err error
-		if loop, err = time.ParseDuration(t); err != nil {
-			respondWith(w, http.StatusBadRequest, t)
-			return
-		}
-	}
-	handleWebsocket(ctx, w, r, rep, loop)
-}
-
 // Individual nodes.
-func handleNode(ctx context.Context, rep Reporter, w http.ResponseWriter, r *http.Request) {
+func handleNode(ctx context.Context, renderer render.Renderer, _ render.Decorator, report report.Report, w http.ResponseWriter, r *http.Request) {
 	var (
-		vars        = mux.Vars(r)
-		topologyID  = vars["topology"]
-		nodeID      = vars["id"]
-		report, err = rep.Report(ctx)
+		vars       = mux.Vars(r)
+		topologyID = vars["topology"]
+		nodeID     = vars["id"]
+		rendered   = renderer.Render(report, nil)
+		node, ok   = rendered[nodeID]
 	)
-	if err != nil {
-		respondWith(w, http.StatusInternalServerError, err.Error())
-		return
-	}
-
-	renderer, _, err := topologyRegistry.rendererForRequest(r, report)
-	if err != nil {
-		http.NotFound(w, r)
-		return
-	}
-
-	var (
-		rendered = renderer.Render(report, nil)
-		node, ok = rendered[nodeID]
-	)
-	if err != nil {
-		respondWith(w, http.StatusInternalServerError, err.Error())
-		return
-	}
 	if !ok {
 		http.NotFound(w, r)
 		return
 	}
 	respondWith(w, http.StatusOK, APINode{Node: detailed.MakeNode(topologyID, report, rendered, node)})
 }
 
+// Websocket for the full topology.
 func handleWebsocket(
 	ctx context.Context,
+	rep Reporter,
 	w http.ResponseWriter,
 	r *http.Request,
-	rep Reporter,
-	loop time.Duration,
 ) {
+	if err := r.ParseForm(); err != nil {
+		respondWith(w, http.StatusInternalServerError, err.Error())
+		return
+	}
+	loop := websocketLoop
+	if t := r.Form.Get("t"); t != "" {
+		var err error
+		if loop, err = time.ParseDuration(t); err != nil {
+			respondWith(w, http.StatusBadRequest, t)
+			return
+		}
+	}
+
 	conn, err := xfer.Upgrade(w, r, nil)
 	if err != nil {
 		// log.Info("Upgrade:", err)
@@ -125,6 +95,7 @@ func handleWebsocket(
 		previousTopo detailed.NodeSummaries
 		tick         = time.Tick(loop)
 		wait         = make(chan struct{}, 1)
+		topologyID   = mux.Vars(r)["topology"]
 	)
 	rep.WaitOn(ctx, wait)
 	defer rep.UnWait(ctx, wait)
@@ -135,7 +106,7 @@ func handleWebsocket(
 			log.Errorf("Error generating report: %v", err)
 			return
 		}
-		renderer, decorator, err := topologyRegistry.rendererForRequest(r, report)
+		renderer, decorator, err := topologyRegistry.rendererForTopology(topologyID, r.Form, report)
 		if err != nil {
 			log.Errorf("Error generating report: %v", err)
 			return

app/router.go

@@ -89,11 +89,11 @@ func RegisterTopologyRoutes(router *mux.Router, r Reporter) {
 	get.HandleFunc("/api/topology",
 		gzipHandler(requestContextDecorator(topologyRegistry.makeTopologyList(r))))
 	get.HandleFunc("/api/topology/{topology}",
-		gzipHandler(requestContextDecorator(captureReporter(r, handleTopology))))
+		gzipHandler(requestContextDecorator(topologyRegistry.captureRenderer(r, handleTopology))))
 	get.HandleFunc("/api/topology/{topology}/ws",
-		requestContextDecorator(captureReporter(r, handleWs))) // NB not gzip!
+		requestContextDecorator(captureReporter(r, handleWebsocket))) // NB not gzip!
 	get.MatcherFunc(URLMatcher("/api/topology/{topology}/{id}")).HandlerFunc(
-		gzipHandler(requestContextDecorator(captureReporter(r, handleNode))))
+		gzipHandler(requestContextDecorator(topologyRegistry.captureRenderer(r, handleNode))))
 	get.HandleFunc("/api/report",
 		gzipHandler(requestContextDecorator(makeRawReportHandler(r))))
 	get.HandleFunc("/api/probes",