gopls: increment the telemetryprompt acceptance counter for each session

Increment the gopls/telemetryprompt/accepted counter every time we read
the prompt file, so that we can actually use the resulting data. We
don't upload any counter files that overlap with periods of time when
telemetry uploading was not enabled, so with the previous model of only
incrementing the counter at the moment the prompt was accepted, we will
never upload the counter file into which the acceptance is recorded.

With the new model, the counter will give us a sense of what fraction of
telemetry participants opted-in as a result of the prompt.

For golang/go#68770

Change-Id: I8890c73b5bfa19023bb24fd156bcaa9eb46295ad
Reviewed-on: https://go-review.googlesource.com/c/tools/+/607758
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/server/prompt.go

@@ -113,15 +113,6 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) {
 		defer work.End(ctx, "Done.")
 	}
 
-	if !enabled { // check this after the work progress message for testing.
-		return // prompt is disabled
-	}
-
-	if s.telemetryMode() == "on" || s.telemetryMode() == "off" {
-		// Telemetry is already on or explicitly off -- nothing to ask about.
-		return
-	}
-
 	errorf := func(format string, args ...any) {
 		err := fmt.Errorf(format, args...)
 		event.Error(ctx, "telemetry prompt failed", err)
@@ -134,12 +125,14 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) {
 		return
 	}
 
+	// Read the current prompt file.
+
 	var (
 		promptDir  = filepath.Join(configDir, "prompt")    // prompt configuration directory
 		promptFile = filepath.Join(promptDir, "telemetry") // telemetry prompt file
 	)
 
-	// prompt states, to be written to the prompt file
+	// prompt states, stored in the prompt file
 	const (
 		pUnknown  = ""        // first time
 		pNotReady = "-"       // user is not asked yet (either not sampled or not past the grace period)
@@ -177,17 +170,55 @@ func (s *server) maybePromptForTelemetry(ctx context.Context, enabled bool) {
 	} else if !os.IsNotExist(err) {
 		errorf("reading prompt file: %v", err)
 		// Something went wrong. Since we don't know how many times we've asked the
-		// prompt, err on the side of not spamming.
+		// prompt, err on the side of not asking.
+		//
+		// But record this in telemetry, in case some users enable telemetry by
+		// other means.
+		counter.New("gopls/telemetryprompt/corrupted").Inc()
 		return
 	}
 
-	// Terminal conditions.
-	if state == pYes || state == pNo {
-		// Prompt has been answered. Nothing to do.
+	counter.New(fmt.Sprintf("gopls/telemetryprompt/attempts:%d", attempts)).Inc()
+
+	// Check terminal conditions.
+
+	if state == pYes {
+		// Prompt has been accepted.
+		//
+		// We record this counter for every gopls session, rather than when the
+		// prompt actually accepted below, because if we only recorded it in the
+		// counter file at the time telemetry is enabled, we'd never upload it,
+		// because we exclude any counter files that overlap with a time period
+		// that has telemetry uploading is disabled.
+		counter.New("gopls/telemetryprompt/accepted").Inc()
+		return
+	}
+	if state == pNo {
+		// Prompt has been declined. In most cases, this means we'll never see the
+		// counter below, but it's possible that the user may enable telemetry by
+		// other means later on. If we see a significant number of users that have
+		// accepted telemetry but declined the prompt, it may be an indication that
+		// the prompt is not working well.
+		counter.New("gopls/telemetryprompt/declined").Inc()
 		return
 	}
 	if attempts >= 5 { // pPending or pFailed
-		// We've tried asking enough; give up.
+		// We've tried asking enough; give up. Record that the prompt expired, in
+		// case the user decides to enable telemetry by other means later on.
+		// (see also the pNo case).
+		counter.New("gopls/telemetryprompt/expired").Inc()
+		return
+	}
+
+	// We only check enabled after (1) the work progress is started, and (2) the
+	// prompt file has been read. (1) is for testing purposes, and (2) is so that
+	// we record the "gopls/telemetryprompt/accepted" counter for every session.
+	if !enabled {
+		return // prompt is disabled
+	}
+
+	if s.telemetryMode() == "on" || s.telemetryMode() == "off" {
+		// Telemetry is already on or explicitly off -- nothing to ask about.
 		return
 	}
 
@@ -309,7 +340,6 @@ Would you like to enable Go telemetry?
 			result = pYes
 			if err := s.setTelemetryMode("on"); err == nil {
 				message(protocol.Info, telemetryOnMessage(s.Options().LinkifyShowMessage))
-				counter.New("gopls/telemetryprompt/accepted").Inc()
 			} else {
 				errorf("enabling telemetry failed: %v", err)
 				msg := fmt.Sprintf("Failed to enable Go telemetry: %v\nTo enable telemetry manually, please run `go run golang.org/x/telemetry/cmd/gotelemetry@latest on`", err)

gopls/internal/test/integration/fake/editor.go

@@ -200,7 +200,8 @@ func (e *Editor) Exit(ctx context.Context) error {
 	return nil
 }
 
-// Close issues the shutdown and exit sequence an editor should.
+// Close disconnects the LSP client session.
+// TODO(rfindley): rename to 'Disconnect'.
 func (e *Editor) Close(ctx context.Context) error {
 	if err := e.Shutdown(ctx); err != nil {
 		return err

gopls/internal/test/integration/fake/workdir.go

@@ -19,6 +19,7 @@ import (
 	"time"
 
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/gopls/internal/util/slices"
 	"golang.org/x/tools/internal/robustio"
 )
 
@@ -333,8 +334,7 @@ func (w *Workdir) CheckForFileChanges(ctx context.Context) error {
 		return nil
 	}
 	w.watcherMu.Lock()
-	watchers := make([]func(context.Context, []protocol.FileEvent), len(w.watchers))
-	copy(watchers, w.watchers)
+	watchers := slices.Clone(w.watchers)
 	w.watcherMu.Unlock()
 	for _, w := range watchers {
 		w(ctx, evts)

gopls/internal/test/integration/misc/prompt_test.go

@@ -13,6 +13,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"golang.org/x/telemetry/counter"
 	"golang.org/x/telemetry/counter/countertest"
 	"golang.org/x/tools/gopls/internal/protocol"
@@ -268,32 +269,63 @@ func main() {
 }
 `
 
-	acceptanceCounterName := "gopls/telemetryprompt/accepted"
-	acceptanceCounter := counter.New(acceptanceCounterName)
-	// We must increment the acceptance counter in order for the initial read
-	// below to succeed.
+	var (
+		acceptanceCounter = "gopls/telemetryprompt/accepted"
+		declinedCounter   = "gopls/telemetryprompt/declined"
+		attempt1Counter   = "gopls/telemetryprompt/attempts:1"
+		allCounters       = []string{acceptanceCounter, declinedCounter, attempt1Counter}
+	)
+
+	// We must increment counters in order for the initial reads below to
+	// succeed.
 	//
 	// TODO(rfindley): ReadCounter should simply return 0 for uninitialized
 	// counters.
-	acceptanceCounter.Inc()
+	for _, name := range allCounters {
+		counter.New(name).Inc()
+	}
+
+	readCounts := func(t *testing.T) map[string]uint64 {
+		t.Helper()
+		counts := make(map[string]uint64)
+		for _, name := range allCounters {
+			count, err := countertest.ReadCounter(counter.New(name))
+			if err != nil {
+				t.Fatalf("ReadCounter(%q) failed: %v", name, err)
+			}
+			counts[name] = count
+		}
+		return counts
+	}
 
 	tests := []struct {
-		name     string // subtest name
-		response string // response to choose for the telemetry dialog
-		wantMode string // resulting telemetry mode
-		wantMsg  string // substring contained in the follow-up popup (if empty, no popup is expected)
-		wantInc  uint64 // expected 'prompt accepted' counter increment
+		name       string // subtest name
+		response   string // response to choose for the telemetry dialog
+		wantMode   string // resulting telemetry mode
+		wantMsg    string // substring contained in the follow-up popup (if empty, no popup is expected)
+		wantInc    uint64 // expected 'prompt accepted' counter increment
+		wantCounts map[string]uint64
 	}{
-		{"yes", server.TelemetryYes, "on", "uploading is now enabled", 1},
-		{"no", server.TelemetryNo, "", "", 0},
-		{"empty", "", "", "", 0},
+		{"yes", server.TelemetryYes, "on", "uploading is now enabled", 1, map[string]uint64{
+			acceptanceCounter: 1,
+			declinedCounter:   0,
+			attempt1Counter:   1,
+		}},
+		{"no", server.TelemetryNo, "", "", 0, map[string]uint64{
+			acceptanceCounter: 0,
+			declinedCounter:   1,
+			attempt1Counter:   1,
+		}},
+		{"empty", "", "", "", 0, map[string]uint64{
+			acceptanceCounter: 0,
+			declinedCounter:   0,
+			attempt1Counter:   1,
+		}},
 	}
+
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			initialCount, err := countertest.ReadCounter(acceptanceCounter)
-			if err != nil {
-				t.Fatalf("ReadCounter(%q) failed: %v", acceptanceCounterName, err)
-			}
+			initialCounts := readCounts(t)
 			modeFile := filepath.Join(t.TempDir(), "mode")
 			telemetryStartTime := time.Now().Add(-8 * 24 * time.Hour)
 			msgRE := regexp.MustCompile(".*Would you like to enable Go telemetry?")
@@ -340,12 +372,22 @@ func main() {
 				if gotMode != test.wantMode {
 					t.Errorf("after prompt, mode=%s, want %s", gotMode, test.wantMode)
 				}
-				finalCount, err := countertest.ReadCounter(acceptanceCounter)
-				if err != nil {
-					t.Fatalf("ReadCounter(%q) failed: %v", acceptanceCounterName, err)
+
+				// We increment the acceptance counter when checking the prompt file
+				// before prompting, so start a second, transient gopls session and
+				// verify that the acceptance counter is incremented.
+				env2 := ConnectGoplsEnv(t, env.Ctx, env.Sandbox, env.Editor.Config(), env.Server)
+				env2.Await(CompletedWork(server.TelemetryPromptWorkTitle, 1, true))
+				if err := env2.Editor.Close(env2.Ctx); err != nil {
+					t.Errorf("closing second editor: %v", err)
+				}
+
+				gotCounts := readCounts(t)
+				for k := range gotCounts {
+					gotCounts[k] -= initialCounts[k]
 				}
-				if gotInc := finalCount - initialCount; gotInc != test.wantInc {
-					t.Errorf("%q mismatch: got %d, want %d", acceptanceCounterName, gotInc, test.wantInc)
+				if diff := cmp.Diff(test.wantCounts, gotCounts); diff != "" {
+					t.Errorf("counter mismatch (-want +got):\n%s", diff)
 				}
 			})
 		})

gopls/internal/test/integration/misc/shared_test.go

@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	. "golang.org/x/tools/gopls/internal/test/integration"
-	"golang.org/x/tools/gopls/internal/test/integration/fake"
 )
 
 // Smoke test that simultaneous editing sessions in the same workspace works.
@@ -32,19 +31,7 @@ func main() {
 	).Run(t, sharedProgram, func(t *testing.T, env1 *Env) {
 		// Create a second test session connected to the same workspace and server
 		// as the first.
-		awaiter := NewAwaiter(env1.Sandbox.Workdir)
-		editor, err := fake.NewEditor(env1.Sandbox, env1.Editor.Config()).Connect(env1.Ctx, env1.Server, awaiter.Hooks())
-		if err != nil {
-			t.Fatal(err)
-		}
-		env2 := &Env{
-			T:       t,
-			Ctx:     env1.Ctx,
-			Sandbox: env1.Sandbox,
-			Server:  env1.Server,
-			Editor:  editor,
-			Awaiter: awaiter,
-		}
+		env2 := ConnectGoplsEnv(t, env1.Ctx, env1.Sandbox, env1.Editor.Config(), env1.Server)
 		env2.Await(InitialWorkspaceLoad)
 		// In editor #1, break fmt.Println as before.
 		env1.OpenFile("main.go")

gopls/internal/test/integration/runner.go

@@ -215,19 +215,7 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio
 			framer = ls.framer(jsonrpc2.NewRawStream)
 			ts := servertest.NewPipeServer(ss, framer)
 
-			awaiter := NewAwaiter(sandbox.Workdir)
-			editor, err := fake.NewEditor(sandbox, config.editor).Connect(ctx, ts, awaiter.Hooks())
-			if err != nil {
-				t.Fatal(err)
-			}
-			env := &Env{
-				T:       t,
-				Ctx:     ctx,
-				Sandbox: sandbox,
-				Editor:  editor,
-				Server:  ts,
-				Awaiter: awaiter,
-			}
+			env := ConnectGoplsEnv(t, ctx, sandbox, config.editor, ts)
 			defer func() {
 				if t.Failed() && r.PrintGoroutinesOnFailure {
 					pprof.Lookup("goroutine").WriteTo(os.Stderr, 1)
@@ -242,7 +230,7 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio
 				// the editor: in general we want to clean up before proceeding to the
 				// next test, and if there is a deadlock preventing closing it will
 				// eventually be handled by the `go test` timeout.
-				if err := editor.Close(xcontext.Detach(ctx)); err != nil {
+				if err := env.Editor.Close(xcontext.Detach(ctx)); err != nil {
 					t.Errorf("closing editor: %v", err)
 				}
 			}()
@@ -253,6 +241,28 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio
 	}
 }
 
+// ConnectGoplsEnv creates a new Gopls environment for the given sandbox,
+// editor config, and server connector.
+//
+// TODO(rfindley): significantly refactor the way testing environments are
+// constructed.
+func ConnectGoplsEnv(t testing.TB, ctx context.Context, sandbox *fake.Sandbox, config fake.EditorConfig, connector servertest.Connector) *Env {
+	awaiter := NewAwaiter(sandbox.Workdir)
+	editor, err := fake.NewEditor(sandbox, config).Connect(ctx, connector, awaiter.Hooks())
+	if err != nil {
+		t.Fatal(err)
+	}
+	env := &Env{
+		T:       t,
+		Ctx:     ctx,
+		Sandbox: sandbox,
+		Server:  connector,
+		Editor:  editor,
+		Awaiter: awaiter,
+	}
+	return env
+}
+
 // longBuilders maps builders that are skipped when -short is set to a
 // (possibly empty) justification.
 var longBuilders = map[string]string{