gopls: enable crashmonitor for unexpected crashes

This change enables the crash monitor in gopls, so that it increments a telemetry counter for the stack any time the Go runtime crashes. x/telemetry cannot be built with go1.18, which gopls still supports (though not for long). So we must ensure that there are no direct references to x/telemetry from gopls; all references must go through the build-tagged non-functional shims, which avoids the dependency. Also, internal/telemetry can no longer depend on protocol, to avoid a cycle via protocol -> bug -> telemetry. Updates golang/go#42888 Change-Id: I7a57b9a93ab76a58ec6dd73f895d4b42ed29ea86 Reviewed-on: https://go-review.googlesource.com/c/tools/+/558256 Reviewed-by: Robert Findley <rfindley@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Auto-Submit: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
golang · Feb 2, 2024 · e211e0f · e211e0f
1 parent d077888
commit e211e0f
Show file tree

Hide file tree

Showing 9 changed files with 102 additions and 70 deletions.
diff --git a/gopls/internal/protocol/generate/main.go b/gopls/internal/protocol/generate/main.go
@@ -100,7 +100,6 @@ func writeclient() {
 		`import (
 	"context"
 
-	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/internal/jsonrpc2"
 )
 `)
@@ -110,12 +109,7 @@ func writeclient() {
 	}
 	out.WriteString("}\n\n")
 	out.WriteString(`func clientDispatch(ctx context.Context, client Client, reply jsonrpc2.Replier, r jsonrpc2.Request) (bool, error) {
-	defer func() {
-		if x := recover(); x != nil {
-			bug.Reportf("client panic in %s request", r.Method())
-			panic(x)
-		}
-	}()
+	defer recoverHandlerPanic(r.Method())
 	switch r.Method() {
 `)
 	for _, k := range ccases.keys() {
@@ -144,7 +138,6 @@ func writeserver() {
 		`import (
 	"context"
 
-	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/internal/jsonrpc2"
 )
 `)
@@ -156,12 +149,7 @@ func writeserver() {
 }
 
 func serverDispatch(ctx context.Context, server Server, reply jsonrpc2.Replier, r jsonrpc2.Request) (bool, error) {
-	defer func() {
-		if x := recover(); x != nil {
-			bug.Reportf("server panic in %s request", r.Method())
-			panic(x)
-		}
-	}()
+	defer recoverHandlerPanic(r.Method())
 	switch r.Method() {
 `)
 	for _, k := range scases.keys() {

diff --git a/gopls/internal/protocol/protocol.go b/gopls/internal/protocol/protocol.go
@@ -11,6 +11,8 @@ import (
 	"fmt"
 	"io"
 
+	"golang.org/x/tools/gopls/internal/telemetry"
+	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/jsonrpc2"
 	jsonrpc2_v2 "golang.org/x/tools/internal/jsonrpc2_v2"
@@ -295,3 +297,17 @@ func NonNilSlice[T comparable](x []T) []T {
 	}
 	return x
 }
+
+func recoverHandlerPanic(method string) {
+	// Report panics in the handler goroutine,
+	// unless we have enabled the monitor,
+	// which reports all crashes.
+	if !telemetry.CrashMonitorSupported() {
+		defer func() {
+			if x := recover(); x != nil {
+				bug.Reportf("panic in %s request", method)
+				panic(x)
+			}
+		}()
+	}
+}
diff --git a/gopls/internal/protocol/tsclient.go b/gopls/internal/protocol/tsclient.go
diff --git a/gopls/internal/protocol/tsserver.go b/gopls/internal/protocol/tsserver.go
diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go
@@ -37,7 +37,11 @@ func (s *server) Initialize(ctx context.Context, params *protocol.ParamInitializ
 	ctx, done := event.Start(ctx, "lsp.Server.initialize")
 	defer done()
 
-	telemetry.RecordClientInfo(params)
+	var clientName string
+	if params != nil && params.ClientInfo != nil {
+		clientName = params.ClientInfo.Name
+	}
+	telemetry.RecordClientInfo(clientName)
 
 	s.stateMu.Lock()
 	if s.state >= serverInitializing {

diff --git a/gopls/internal/telemetry/telemetry.go b/gopls/internal/telemetry/telemetry.go
@@ -12,10 +12,30 @@ import (
 
 	"golang.org/x/telemetry"
 	"golang.org/x/telemetry/counter"
+	"golang.org/x/telemetry/crashmonitor"
 	"golang.org/x/telemetry/upload"
-	"golang.org/x/tools/gopls/internal/protocol"
 )
 
+// CounterOpen calls [counter.Open].
+func CounterOpen() {
+	counter.Open()
+}
+
+// StartCrashMonitor calls [crashmonitor.Start].
+func StartCrashMonitor() {
+	crashmonitor.Start()
+}
+
+// CrashMonitorSupported calls [crashmonitor.Supported].
+func CrashMonitorSupported() bool {
+	return crashmonitor.Supported()
+}
+
+// NewStackCounter calls [counter.NewStack].
+func NewStackCounter(name string, depth int) *counter.StackCounter {
+	return counter.NewStack(name, depth)
+}
+
 // Mode calls x/telemetry.Mode.
 func Mode() string {
 	return telemetry.Mode()
@@ -32,41 +52,42 @@ func Upload() {
 }
 
 // RecordClientInfo records gopls client info.
-func RecordClientInfo(params *protocol.ParamInitialize) {
-	client := "gopls/client:other"
-	if params != nil && params.ClientInfo != nil {
-		switch params.ClientInfo.Name {
-		case "Visual Studio Code":
-			client = "gopls/client:vscode"
-		case "Visual Studio Code - Insiders":
-			client = "gopls/client:vscode-insiders"
-		case "VSCodium":
-			client = "gopls/client:vscodium"
-		case "code-server":
-			// https://github.com/coder/code-server/blob/3cb92edc76ecc2cfa5809205897d93d4379b16a6/ci/build/build-vscode.sh#L19
-			client = "gopls/client:code-server"
-		case "Eglot":
-			// https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-03/msg00954.html
-			client = "gopls/client:eglot"
-		case "govim":
-			// https://github.com/govim/govim/pull/1189
-			client = "gopls/client:govim"
-		case "Neovim":
-			// https://github.com/neovim/neovim/blob/42333ea98dfcd2994ee128a3467dfe68205154cd/runtime/lua/vim/lsp.lua#L1361
-			client = "gopls/client:neovim"
-		case "coc.nvim":
-			// https://github.com/neoclide/coc.nvim/blob/3dc6153a85ed0f185abec1deb972a66af3fbbfb4/src/language-client/client.ts#L994
-			client = "gopls/client:coc.nvim"
-		case "Sublime Text LSP":
-			// https://github.com/sublimelsp/LSP/blob/e608f878e7e9dd34aabe4ff0462540fadcd88fcc/plugin/core/sessions.py#L493
-			client = "gopls/client:sublimetext"
-		default:
-			// at least accumulate the client name locally
-			counter.New(fmt.Sprintf("gopls/client-other:%s", params.ClientInfo.Name)).Inc()
-			// but also record client:other
+func RecordClientInfo(clientName string) {
+	key := "gopls/client:other"
+	switch clientName {
+	case "Visual Studio Code":
+		key = "gopls/client:vscode"
+	case "Visual Studio Code - Insiders":
+		key = "gopls/client:vscode-insiders"
+	case "VSCodium":
+		key = "gopls/client:vscodium"
+	case "code-server":
+		// https://github.com/coder/code-server/blob/3cb92edc76ecc2cfa5809205897d93d4379b16a6/ci/build/build-vscode.sh#L19
+		key = "gopls/client:code-server"
+	case "Eglot":
+		// https://lists.gnu.org/archive/html/bug-gnu-emacs/2023-03/msg00954.html
+		key = "gopls/client:eglot"
+	case "govim":
+		// https://github.com/govim/govim/pull/1189
+		key = "gopls/client:govim"
+	case "Neovim":
+		// https://github.com/neovim/neovim/blob/42333ea98dfcd2994ee128a3467dfe68205154cd/runtime/lua/vim/lsp.lua#L1361
+		key = "gopls/client:neovim"
+	case "coc.nvim":
+		// https://github.com/neoclide/coc.nvim/blob/3dc6153a85ed0f185abec1deb972a66af3fbbfb4/src/language-client/client.ts#L994
+		key = "gopls/client:coc.nvim"
+	case "Sublime Text LSP":
+		// https://github.com/sublimelsp/LSP/blob/e608f878e7e9dd34aabe4ff0462540fadcd88fcc/plugin/core/sessions.py#L493
+		key = "gopls/client:sublimetext"
+	default:
+		// Accumulate at least a local counter for an unknown
+		// client name, but also fall through to count it as
+		// ":other" for collection.
+		if clientName != "" {
+			counter.New(fmt.Sprintf("gopls/client-other:%s", clientName)).Inc()
 		}
 	}
-	counter.Inc(client)
+	counter.Inc(key)
 }
 
 // RecordViewGoVersion records the Go minor version number (1.x) used for a view.

diff --git a/gopls/internal/telemetry/telemetry_go118.go b/gopls/internal/telemetry/telemetry_go118.go
@@ -7,7 +7,22 @@
 
 package telemetry
 
-import "golang.org/x/tools/gopls/internal/protocol"
+// This file defines dummy implementations of telemetry operations to
+// permit building with go1.18. Until we drop support for go1.18,
+// gopls may not refer to the telemetry module directly, but must go
+// through this file.
+
+func CounterOpen() {}
+
+func StartCrashMonitor() {}
+
+func CrashMonitorSupported() bool { return false }
+
+func NewStackCounter(string, int) dummyCounter { return dummyCounter{} }
+
+type dummyCounter struct{}
+
+func (dummyCounter) Inc() {}
 
 func Mode() string {
 	return "local"
@@ -20,8 +35,7 @@ func SetMode(mode string) error {
 func Upload() {
 }
 
-func RecordClientInfo(params *protocol.ParamInitialize) {
-}
+func RecordClientInfo(string) {}
 
 func RecordViewGoVersion(x int) {
 }

diff --git a/gopls/internal/util/bug/bug.go b/gopls/internal/util/bug/bug.go
@@ -19,7 +19,7 @@ import (
 	"sync"
 	"time"
 
-	"golang.org/x/telemetry/counter"
+	"golang.org/x/tools/gopls/internal/telemetry"
 )
 
 // PanicOnBugs controls whether to panic when bugs are reported.
@@ -66,7 +66,7 @@ func Report(description string) {
 }
 
 // BugReportCount is a telemetry counter that tracks # of bug reports.
-var BugReportCount = counter.NewStack("gopls/bug", 16)
+var BugReportCount = telemetry.NewStackCounter("gopls/bug", 16)
 
 func report(description string) {
 	_, file, line, ok := runtime.Caller(2) // all exported reporting functions call report directly

diff --git a/gopls/main.go b/gopls/main.go
@@ -17,9 +17,9 @@ import (
 	"context"
 	"os"
 
-	"golang.org/x/telemetry/counter"
 	"golang.org/x/tools/gopls/internal/cmd"
 	"golang.org/x/tools/gopls/internal/hooks"
+	"golang.org/x/tools/gopls/internal/telemetry"
 	versionpkg "golang.org/x/tools/gopls/internal/version"
 	"golang.org/x/tools/internal/tool"
 )
@@ -29,7 +29,8 @@ var version = "" // if set by the linker, overrides the gopls version
 func main() {
 	versionpkg.VersionOverride = version
 
-	counter.Open() // Enable telemetry counter writing.
+	telemetry.CounterOpen()
+	telemetry.StartCrashMonitor()
 	ctx := context.Background()
 	tool.Main(ctx, cmd.New(hooks.Options), os.Args[1:])
 }