From 8421a35b9ef4b19c2cb517671c48c9154ee51841 Mon Sep 17 00:00:00 2001 From: "Hana (Hyang-Ah) Kim" Date: Wed, 13 Sep 2023 12:15:37 -0400 Subject: [PATCH] gopls/lsp/command: add gopls.add_telemetry_counters Gopls clients can ask gopls to increment the specified counters using this custom command. Other than communication errors or obvious violation of protocol (e.g. unmatched names/values length, type errors), this will never return an error even when some counters cannot be updated. golang.org/x/telemetry/counter APIs does not report errors. Change-Id: I21dbbba461f04819df8a7b2190fbe77547375d99 Reviewed-on: https://go-review.googlesource.com/c/tools/+/527821 Run-TryBot: Hyang-Ah Hana Kim TryBot-Result: Gopher Robot Reviewed-by: Robert Findley --- gopls/doc/commands.md | 16 ++++++++++++++ gopls/internal/lsp/command.go | 11 ++++++++++ gopls/internal/lsp/command/command_gen.go | 20 +++++++++++++++++ gopls/internal/lsp/command/interface.go | 14 ++++++++++++ gopls/internal/lsp/source/api_json.go | 6 ++++++ gopls/internal/telemetry/telemetry.go | 12 +++++++++++ gopls/internal/telemetry/telemetry_go118.go | 3 +++ gopls/internal/telemetry/telemetry_test.go | 24 ++++++++++++++++++++- 8 files changed, 105 insertions(+), 1 deletion(-) diff --git a/gopls/doc/commands.md b/gopls/doc/commands.md index 1a8c4dc99..a2ffda56a 100644 --- a/gopls/doc/commands.md +++ b/gopls/doc/commands.md @@ -41,6 +41,22 @@ Args: } ``` +### **update the given telemetry counters.** +Identifier: `gopls.add_telemetry_counters` + +Gopls will prepend "fwd/" to all the counters updated using this command +to avoid conflicts with other counters gopls collects. + +Args: + +``` +{ + // Names and Values must have the same length. + "Names": []string, + "Values": []int64, +} +``` + ### **Apply a fix** Identifier: `gopls.apply_fix` diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go index 99b0b384f..a64007b98 100644 --- a/gopls/internal/lsp/command.go +++ b/gopls/internal/lsp/command.go @@ -28,6 +28,7 @@ import ( "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/gopls/internal/telemetry" "golang.org/x/tools/gopls/internal/vulncheck" "golang.org/x/tools/gopls/internal/vulncheck/scan" "golang.org/x/tools/internal/event" @@ -63,6 +64,16 @@ type commandHandler struct { params *protocol.ExecuteCommandParams } +// AddTelemetryCounters implements command.Interface. +func (*commandHandler) AddTelemetryCounters(_ context.Context, args command.AddTelemetryCountersArgs) error { + if len(args.Names) != len(args.Values) { + return fmt.Errorf("Names and Values must have the same length") + } + // invalid counter update requests will be silently dropped. (no audience) + telemetry.AddForwardedCounters(args.Names, args.Values) + return nil +} + // commandConfig configures common command set-up and execution. type commandConfig struct { async bool // whether to run the command asynchronously. Async commands can only return errors. diff --git a/gopls/internal/lsp/command/command_gen.go b/gopls/internal/lsp/command/command_gen.go index 00b765796..40eda2781 100644 --- a/gopls/internal/lsp/command/command_gen.go +++ b/gopls/internal/lsp/command/command_gen.go @@ -21,6 +21,7 @@ import ( const ( AddDependency Command = "add_dependency" AddImport Command = "add_import" + AddTelemetryCounters Command = "add_telemetry_counters" ApplyFix Command = "apply_fix" CheckUpgrades Command = "check_upgrades" EditGoDirective Command = "edit_go_directive" @@ -52,6 +53,7 @@ const ( var Commands = []Command{ AddDependency, AddImport, + AddTelemetryCounters, ApplyFix, CheckUpgrades, EditGoDirective, @@ -94,6 +96,12 @@ func Dispatch(ctx context.Context, params *protocol.ExecuteCommandParams, s Inte return nil, err } return nil, s.AddImport(ctx, a0) + case "gopls.add_telemetry_counters": + var a0 AddTelemetryCountersArgs + if err := UnmarshalArgs(params.Arguments, &a0); err != nil { + return nil, err + } + return nil, s.AddTelemetryCounters(ctx, a0) case "gopls.apply_fix": var a0 ApplyFixArgs if err := UnmarshalArgs(params.Arguments, &a0); err != nil { @@ -272,6 +280,18 @@ func NewAddImportCommand(title string, a0 AddImportArgs) (protocol.Command, erro }, nil } +func NewAddTelemetryCountersCommand(title string, a0 AddTelemetryCountersArgs) (protocol.Command, error) { + args, err := MarshalArgs(a0) + if err != nil { + return protocol.Command{}, err + } + return protocol.Command{ + Title: title, + Command: "gopls.add_telemetry_counters", + Arguments: args, + }, nil +} + func NewApplyFixCommand(title string, a0 ApplyFixArgs) (protocol.Command, error) { args, err := MarshalArgs(a0) if err != nil { diff --git a/gopls/internal/lsp/command/interface.go b/gopls/internal/lsp/command/interface.go index 261776d5a..2ae50fb0e 100644 --- a/gopls/internal/lsp/command/interface.go +++ b/gopls/internal/lsp/command/interface.go @@ -189,6 +189,12 @@ type Interface interface { // RunGoWorkCommand: run `go work [args...]`, and apply the resulting go.work // edits to the current go.work file. RunGoWorkCommand(context.Context, RunGoWorkArgs) error + + // AddTelemetryCounters: update the given telemetry counters. + // + // Gopls will prepend "fwd/" to all the counters updated using this command + // to avoid conflicts with other counters gopls collects. + AddTelemetryCounters(context.Context, AddTelemetryCountersArgs) error } type RunTestsArgs struct { @@ -499,3 +505,11 @@ type RunGoWorkArgs struct { InitFirst bool // Whether to run `go work init` first Args []string // Args to pass to `go work` } + +// AddTelemetryCountersArgs holds the arguments to the AddCounters command +// that updates the telemetry counters. +type AddTelemetryCountersArgs struct { + // Names and Values must have the same length. + Names []string // Name of counters. + Values []int64 // Values added to the corresponding counters. Must be non-negative. +} diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go index 4d85ed7b5..d0fdb2008 100644 --- a/gopls/internal/lsp/source/api_json.go +++ b/gopls/internal/lsp/source/api_json.go @@ -709,6 +709,12 @@ var GeneratedAPIJSON = &APIJSON{ Doc: "Ask the server to add an import path to a given Go file. The method will\ncall applyEdit on the client so that clients don't have to apply the edit\nthemselves.", ArgDoc: "{\n\t// ImportPath is the target import path that should\n\t// be added to the URI file\n\t\"ImportPath\": string,\n\t// URI is the file that the ImportPath should be\n\t// added to\n\t\"URI\": string,\n}", }, + { + Command: "gopls.add_telemetry_counters", + Title: "update the given telemetry counters.", + Doc: "Gopls will prepend \"fwd/\" to all the counters updated using this command\nto avoid conflicts with other counters gopls collects.", + ArgDoc: "{\n\t// Names and Values must have the same length.\n\t\"Names\": []string,\n\t\"Values\": []int64,\n}", + }, { Command: "gopls.apply_fix", Title: "Apply a fix", diff --git a/gopls/internal/telemetry/telemetry.go b/gopls/internal/telemetry/telemetry.go index db75e1a7f..840896d51 100644 --- a/gopls/internal/telemetry/telemetry.go +++ b/gopls/internal/telemetry/telemetry.go @@ -68,3 +68,15 @@ func RecordViewGoVersion(x int) { name := fmt.Sprintf("gopls/goversion:1.%d", x) counter.Inc(name) } + +// AddForwardedCounters adds the given counters on behalf of clients. +// Names and values must have the same length. +func AddForwardedCounters(names []string, values []int64) { + for i, n := range names { + v := values[i] + if n == "" || v < 0 { + continue // Should we report an error? Who is the audience? + } + counter.Add("fwd/"+n, v) + } +} diff --git a/gopls/internal/telemetry/telemetry_go118.go b/gopls/internal/telemetry/telemetry_go118.go index b0c1197cb..2d1d60404 100644 --- a/gopls/internal/telemetry/telemetry_go118.go +++ b/gopls/internal/telemetry/telemetry_go118.go @@ -17,3 +17,6 @@ func RecordClientInfo(params *protocol.ParamInitialize) { func RecordViewGoVersion(x int) { } + +func AddForwardedCounters(names []string, values []int64) { +} diff --git a/gopls/internal/telemetry/telemetry_test.go b/gopls/internal/telemetry/telemetry_test.go index 93751bff1..57fff1c47 100644 --- a/gopls/internal/telemetry/telemetry_test.go +++ b/gopls/internal/telemetry/telemetry_test.go @@ -18,6 +18,8 @@ import ( "golang.org/x/telemetry/counter/countertest" // requires go1.21+ "golang.org/x/tools/gopls/internal/bug" "golang.org/x/tools/gopls/internal/hooks" + "golang.org/x/tools/gopls/internal/lsp/command" + "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" ) @@ -43,8 +45,9 @@ func TestTelemetry(t *testing.T) { Modes(Default), // must be in-process to receive the bug report below Settings{"showBugReports": true}, ClientName("Visual Studio Code"), - ).Run(t, "", func(t *testing.T, env *Env) { + ).Run(t, "", func(_ *testing.T, env *Env) { goversion = strconv.Itoa(env.GoVersion()) + addForwardedCounters(env, []string{"vscode/linter:a"}, []int64{1}) const desc = "got a bug" bug.Report(desc) // want a stack counter with the trace starting from here. env.Await(ShownMessage(desc)) @@ -52,9 +55,11 @@ func TestTelemetry(t *testing.T) { // gopls/editor:client // gopls/goversion:1.x + // fwd/vscode/linter:a for _, c := range []*counter.Counter{ counter.New("gopls/client:" + editor), counter.New("gopls/goversion:1." + goversion), + counter.New("fwd/vscode/linter:a"), } { count, err := countertest.ReadCounter(c) if err != nil || count != 1 { @@ -75,6 +80,23 @@ func TestTelemetry(t *testing.T) { } } +func addForwardedCounters(env *Env, names []string, values []int64) { + args, err := command.MarshalArgs(command.AddTelemetryCountersArgs{ + Names: names, Values: values, + }) + if err != nil { + env.T.Fatal(err) + } + var res error + env.ExecuteCommand(&protocol.ExecuteCommandParams{ + Command: command.AddTelemetryCounters.ID(), + Arguments: args, + }, res) + if res != nil { + env.T.Errorf("%v failed - %v", command.AddTelemetryCounters.ID(), res) + } +} + func hasEntry(counts map[string]uint64, pattern string, want uint64) bool { for k, v := range counts { if strings.Contains(k, pattern) && v == want {