gopls/internal/server: redo completion counters

Rearrange the counters so that there is one set describing the
set of suggested completions (empty, short, long) and one set
describing what the user does (used, not used, can't tell).

A set of suggested completions is long if there are more than ten
of them, which is about how many clients show to users.

If a client sends back changes to file, gopls can decide if a
completion was used or not, but it does not try to tell if the client
sends back a new whole file.

These sets of counters correspond to two partitions.

Change-Id: I861a5743b16e488074fd2e58cb63c6a8aea52735
Reviewed-on: https://go-review.googlesource.com/c/tools/+/572315
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: pjweinbgo

gopls/internal/server/completion.go

@@ -40,7 +40,6 @@ func (s *server) Completion(ctx context.Context, params *protocol.CompletionPara
 	var surrounding *completion.Selection
 	switch snapshot.FileKind(fh) {
 	case file.Go:
-		complCnt.Inc() // completion requests for Go programs
 		candidates, surrounding, err = completion.Completion(ctx, snapshot, fh, params.Position, params.Context)
 	case file.Mod:
 		candidates, surrounding = nil, nil
@@ -87,6 +86,8 @@ func (s *server) Completion(ctx context.Context, params *protocol.CompletionPara
 	if len(items) > 10 {
 		// TODO(pjw): long completions are ok for field lists
 		complLong.Inc()
+	} else {
+		complShort.Inc()
 	}
 	return &protocol.CompletionList{
 		IsIncomplete: incompleteResults,
@@ -97,6 +98,7 @@ func (s *server) Completion(ctx context.Context, params *protocol.CompletionPara
 func (s *server) saveLastCompletion(uri protocol.DocumentURI, version int32, items []protocol.CompletionItem, pos protocol.Position) {
 	s.efficacyMu.Lock()
 	defer s.efficacyMu.Unlock()
+	s.efficacyVersion = version
 	s.efficacyURI = uri
 	s.efficacyPos = pos
 	s.efficacyItems = items

gopls/internal/server/counters.go

@@ -8,22 +8,21 @@ import "golang.org/x/telemetry/counter"
 
 // Proposed counters for evaluating gopls code completion.
 var (
-	complCnt   = counter.New("gopls/completion/cnt")   // for Go programs
-	complEmpty = counter.New("gopls/completion/empty") // count empty responses
-	complLong  = counter.New("gopls/completion/long")  // returning more than 10 items
+	complEmpty = counter.New("gopls/completion/len:0")    // count empty suggestions
+	complShort = counter.New("gopls/completion/len:<=10") // not empty, not long
+	complLong  = counter.New("gopls/completion/len:>10")  // returning more than 10 items
 
-	changeMulti = counter.New("gopls/completion/multi-change") // multiple changes in didChange
-	changeFull  = counter.New("gopls/completion/full-change")  // full file change in didChange
+	changeFull  = counter.New("gopls/completion/used:unknown") // full file change in didChange
+	complUnused = counter.New("gopls/completion/used:no")      // did not use a completion
+	complUsed   = counter.New("gopls/completion/used:yes")     // used a completion
 
-	complUsed = counter.New("gopls/completion/used") // used a completion
-
-	// exported so tests can verify that counters are incrementd
+	// exported so tests can verify that counters are incremented
 	CompletionCounters = []*counter.Counter{
-		complCnt,
 		complEmpty,
+		complShort,
 		complLong,
-		changeMulti,
 		changeFull,
+		complUnused,
 		complUsed,
 	}
 )

gopls/internal/server/text_synchronization.go

@@ -329,9 +329,6 @@ func (s *server) applyIncrementalChanges(ctx context.Context, uri protocol.Docum
 	if err != nil {
 		return nil, fmt.Errorf("%w: file not found (%v)", jsonrpc2.ErrInternal, err)
 	}
-	if len(changes) > 1 {
-		changeMulti.Inc()
-	}
 	for i, change := range changes {
 		// TODO(adonovan): refactor to use diff.Apply, which is robust w.r.t.
 		// out-of-order or overlapping changes---and much more efficient.
@@ -396,6 +393,7 @@ func (s *server) checkEfficacy(uri protocol.DocumentURI, version int32, change p
 			}
 		}
 	}
+	complUnused.Inc()
 }
 
 func changeTypeToFileAction(ct protocol.FileChangeType) file.Action {

gopls/internal/test/integration/completion/completion_test.go

@@ -1003,9 +1003,8 @@ func Join() {}
 	})
 }
 
-// show that the counters get exercised. Fortuntely a small program
-// exercises them all (except for mulit-chnage, for which see the code
-// in env.setBufferContentLocked)
+// show that the efficacy counters get exercised. Fortuntely a small program
+// exercises them all
 func TestCounters(t *testing.T) {
 	const files = `
 -- go.mod --
@@ -1028,33 +1027,36 @@ func main() {
 			}
 			return ans
 		}
+		before := cts()
 		env.OpenFile("x.go")
 		env.Await(env.DoneWithOpen())
 		saved := env.BufferText("x.go")
 		lines := strings.Split(saved, "\n")
-		loc := env.RegexpSearch("x.go", "p")
-		before := cts()
+		// make sure the unused counter is exercised
+		loc := env.RegexpSearch("x.go", "main")
+		loc.Range.End = loc.Range.Start
+		env.Completion(loc)                       // ignore the proposed completions
+		env.RegexpReplace("x.go", "main", "Main") // completions are unused
+		env.SetBufferContent("x.go", saved)       // restore x.go
+		// used:no
+
 		// all the action is after 4 characters on line 2 (counting from 0)
-		// (doing the whole file is too expensive)
 		for i := 2; i < len(lines); i++ {
 			l := lines[i]
 			loc.Range.Start.Line = uint32(i)
-			for j := 5; j < len(l); j++ {
+			for j := 4; j < len(l); j++ {
 				loc.Range.Start.Character = uint32(j)
 				loc.Range.End = loc.Range.Start
 				res := env.Completion(loc)
-				for _, r := range res.Items {
+				if len(res.Items) > 0 {
+					r := res.Items[0]
 					env.AcceptCompletion(loc, r)
 					env.SetBufferContent("x.go", saved)
 				}
 			}
 		}
 		after := cts()
 		for c := range after {
-			if c.Name() == "gopls/completion/multi-change" {
-				// don't know how to force multi-change
-				continue
-			}
 			if after[c] <= before[c] {
 				t.Errorf("%s did not increase", c.Name())
 			}