internal/lsp: move all debugging support to the debug package

We now carry around a debug instance in the app, and it manages the log file and
all telemetry

Change-Id: If4a51a36c38ef301b1b2bbda8e4c0a8d9bfc3c04
Reviewed-on: https://go-review.googlesource.com/c/tools/+/218457
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

internal/lsp/cmd/cmd.go

@@ -18,15 +18,15 @@ import (
 	"os"
 	"strings"
 	"sync"
+	"time"
 
 	"golang.org/x/tools/internal/jsonrpc2"
 	"golang.org/x/tools/internal/lsp"
 	"golang.org/x/tools/internal/lsp/cache"
+	"golang.org/x/tools/internal/lsp/debug"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
-	"golang.org/x/tools/internal/telemetry/export"
-	"golang.org/x/tools/internal/telemetry/export/ocagent"
 	"golang.org/x/tools/internal/tool"
 	"golang.org/x/tools/internal/xcontext"
 	errors "golang.org/x/xerrors"
@@ -69,6 +69,8 @@ type Application struct {
 	// PrepareOptions is called to update the options when a new view is built.
 	// It is primarily to allow the behavior of gopls to be modified by hooks.
 	PrepareOptions func(*source.Options)
+
+	debug debug.Instance
 }
 
 // New returns a new Application ready to run.
@@ -130,10 +132,12 @@ gopls flags are:
 // If no arguments are passed it will invoke the server sub command, as a
 // temporary measure for compatibility.
 func (app *Application) Run(ctx context.Context, args ...string) error {
-	ocConfig := ocagent.Discover()
+	app.debug = debug.Instance{
-	//TODO: we should not need to adjust the discovered configuration
+		StartTime:     time.Now(),
-	ocConfig.Address = app.OCAgent
+		Workdir:       app.wd,
-	export.AddExporters(ocagent.Connect(ocConfig))
+		OCAgentConfig: app.OCAgent,
+	}
+	app.debug.Prepare(ctx)
 	app.Serve.app = app
 	if len(args) == 0 {
 		return tool.Run(ctx, &app.Serve, args)

internal/lsp/cmd/serve.go

@@ -9,19 +9,14 @@ import (
 	"flag"
 	"fmt"
 	"io"
-	"log"
 	"net"
 	"os"
-	"path/filepath"
-	"time"
 
 	"golang.org/x/tools/internal/jsonrpc2"
 	"golang.org/x/tools/internal/lsp/cache"
-	"golang.org/x/tools/internal/lsp/debug"
 	"golang.org/x/tools/internal/lsp/lsprpc"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/tool"
-	errors "golang.org/x/xerrors"
 )
 
 // Serve is a struct that exposes the configurable parts of the LSP server as
@@ -58,29 +53,13 @@ func (s *Serve) Run(ctx context.Context, args ...string) error {
 	if len(args) > 0 {
 		return tool.CommandLineErrorf("server does not take arguments, got %v", args)
 	}
-	out := os.Stderr
-	logfile := s.Logfile
-	if logfile != "" {
-		if logfile == "auto" {
-			logfile = filepath.Join(os.TempDir(), fmt.Sprintf("gopls-%d.log", os.Getpid()))
-		}
-		f, err := os.Create(logfile)
-		if err != nil {
-			return errors.Errorf("Unable to create log file: %v", err)
-		}
-		defer f.Close()
-		log.SetOutput(io.MultiWriter(os.Stderr, f))
-		out = f
-	}
 
-	debug := debug.Instance{
+	if err := s.app.debug.SetLogFile(s.Logfile); err != nil {
-		Logfile:       logfile,
+		return err
-		StartTime:     time.Now(),
-		ServerAddress: s.Address,
-		DebugAddress:  s.Debug,
-		Workdir:       s.app.wd,
 	}
-	debug.Serve(ctx, s.Debug)
+	s.app.debug.ServerAddress = s.Address
+	s.app.debug.DebugAddress = s.Debug
+	s.app.debug.Serve(ctx)
 
 	if s.app.Remote != "" {
 		return s.forward()
@@ -96,7 +75,7 @@ func (s *Serve) Run(ctx context.Context, args ...string) error {
 	}
 	stream := jsonrpc2.NewHeaderStream(os.Stdin, os.Stdout)
 	if s.Trace {
-		stream = protocol.LoggingStream(stream, out)
+		stream = protocol.LoggingStream(stream, s.app.debug.LogWriter)
 	}
 	return ss.ServeStream(ctx, stream)
 }

internal/lsp/debug/serve.go

@@ -7,14 +7,18 @@ package debug
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"go/token"
 	"html/template"
+	"io"
 	stdlog "log"
 	"net"
 	"net/http"
 	"net/http/pprof"
 	_ "net/http/pprof" // pull in the standard pprof handlers
+	"os"
 	"path"
+	"path/filepath"
 	"reflect"
 	"runtime"
 	"strconv"
@@ -24,6 +28,7 @@ import (
 
 	"golang.org/x/tools/internal/span"
 	"golang.org/x/tools/internal/telemetry/export"
+	"golang.org/x/tools/internal/telemetry/export/ocagent"
 	"golang.org/x/tools/internal/telemetry/export/prometheus"
 	"golang.org/x/tools/internal/telemetry/log"
 	"golang.org/x/tools/internal/telemetry/tag"
@@ -35,6 +40,14 @@ type Instance struct {
 	ServerAddress string
 	DebugAddress  string
 	Workdir       string
+	OCAgentConfig string
+
+	LogWriter io.Writer
+
+	ocagent    export.Exporter
+	prometheus *prometheus.Exporter
+	rpcs       *rpcs
+	traces     *traces
 }
 
 type Cache interface {
@@ -229,29 +242,57 @@ func DropView(view View) {
 	}
 }
 
+// Prepare gets a debug instance ready for use using the supplied configuration.
+func (i *Instance) Prepare(ctx context.Context) {
+	mu.Lock()
+	defer mu.Unlock()
+	i.LogWriter = os.Stderr
+	ocConfig := ocagent.Discover()
+	//TODO: we should not need to adjust the discovered configuration
+	ocConfig.Address = i.OCAgentConfig
+	i.ocagent = ocagent.Connect(ocConfig)
+	i.prometheus = prometheus.New()
+	i.rpcs = &rpcs{}
+	i.traces = &traces{}
+	export.AddExporters(i.ocagent, i.prometheus, i.rpcs, i.traces)
+}
+
+func (i *Instance) SetLogFile(logfile string) error {
+	if logfile != "" {
+		if logfile == "auto" {
+			logfile = filepath.Join(os.TempDir(), fmt.Sprintf("gopls-%d.log", os.Getpid()))
+		}
+		f, err := os.Create(logfile)
+		if err != nil {
+			return fmt.Errorf("Unable to create log file: %v", err)
+		}
+		defer f.Close()
+		stdlog.SetOutput(io.MultiWriter(os.Stderr, f))
+		i.LogWriter = f
+	}
+	i.Logfile = logfile
+	return nil
+}
+
 // Serve starts and runs a debug server in the background.
 // It also logs the port the server starts on, to allow for :0 auto assigned
 // ports.
-func (i *Instance) Serve(ctx context.Context, addr string) error {
+func (i *Instance) Serve(ctx context.Context) error {
 	mu.Lock()
 	defer mu.Unlock()
-	if addr == "" {
+	if i.DebugAddress == "" {
 		return nil
 	}
-	listener, err := net.Listen("tcp", addr)
+	listener, err := net.Listen("tcp", i.DebugAddress)
 	if err != nil {
 		return err
 	}
 
 	port := listener.Addr().(*net.TCPAddr).Port
-	if strings.HasSuffix(addr, ":0") {
+	if strings.HasSuffix(i.DebugAddress, ":0") {
 		stdlog.Printf("debug server listening on port %d", port)
 	}
 	log.Print(ctx, "Debug serving", tag.Of("Port", port))
-	prometheus := prometheus.New()
-	rpcs := &rpcs{}
-	traces := &traces{}
-	export.AddExporters(prometheus, rpcs, traces)
 	go func() {
 		mux := http.NewServeMux()
 		mux.HandleFunc("/", render(mainTmpl, func(*http.Request) interface{} { return data }))
@@ -261,9 +302,15 @@ func (i *Instance) Serve(ctx context.Context, addr string) error {
 		mux.HandleFunc("/debug/pprof/profile", pprof.Profile)
 		mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
 		mux.HandleFunc("/debug/pprof/trace", pprof.Trace)
-		mux.HandleFunc("/metrics/", prometheus.Serve)
+		if i.prometheus != nil {
-		mux.HandleFunc("/rpc/", render(rpcTmpl, rpcs.getData))
+			mux.HandleFunc("/metrics/", i.prometheus.Serve)
-		mux.HandleFunc("/trace/", render(traceTmpl, traces.getData))
+		}
+		if i.rpcs != nil {
+			mux.HandleFunc("/rpc/", render(rpcTmpl, i.rpcs.getData))
+		}
+		if i.traces != nil {
+			mux.HandleFunc("/trace/", render(traceTmpl, i.traces.getData))
+		}
 		mux.HandleFunc("/cache/", render(cacheTmpl, getCache))
 		mux.HandleFunc("/session/", render(sessionTmpl, getSession))
 		mux.HandleFunc("/view/", render(viewTmpl, getView))