From 33710607cc7c5f94cc06d864c40acc2bc0037e65 Mon Sep 17 00:00:00 2001 From: Paul Bellamy Date: Tue, 3 May 2016 11:59:31 +0100 Subject: [PATCH] Review feedback --- app/api_topologies.go | 52 ++++++++++++++++----------- app/api_topology.go | 81 ++++++++++++++----------------------------- app/router.go | 6 ++-- 3 files changed, 61 insertions(+), 78 deletions(-) diff --git a/app/api_topologies.go b/app/api_topologies.go index adf2611129..ee02ad3ebd 100644 --- a/app/api_topologies.go +++ b/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) } } diff --git a/app/api_topology.go b/app/api_topology.go index da55bdd58e..3a6eabb89d 100644 --- a/app/api_topology.go +++ b/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,66 +29,21 @@ 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 @@ -94,13 +51,26 @@ func handleNode(ctx context.Context, rep Reporter, w http.ResponseWriter, r *htt 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 diff --git a/app/router.go b/app/router.go index d8467f4e79..960717ba5a 100644 --- a/app/router.go +++ b/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",