Skip to content

Commit

Permalink
internal/lsp/cmd: add a flag to disable telemetry
Browse files Browse the repository at this point in the history
govim integration tests (and probably some real user sessions) are
broken because telemetry metrics are not threadsafe, resulting in an
index out of range panic.

Fix this by adding a flag (labeled temporary) to disable telemetry
export.

Also temporarily update govim to master to pick up some fixes, and run
only the -short tests to avoid timeouts.

Updates golang/go#38042

Change-Id: I584e5d200c2f732bd4024002ee6253d09623b29f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/226057
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Cottrell <[email protected]>
findleyr committed Mar 27, 2020
1 parent 8f81e2e commit 17a19b5
Showing 3 changed files with 14 additions and 2 deletions.
2 changes: 1 addition & 1 deletion gopls/integration/govim/Dockerfile
Original file line number Diff line number Diff line change
@@ -17,4 +17,4 @@ WORKDIR /src
# a redundant copy of govim to the build cache using `go mod download`.
RUN GOVIM_VERSION=$(go mod download -json github.com/govim/govim@latest | jq -r .Version) && \
git clone https://github.com/govim/govim /src/govim && cd /src/govim && \
git checkout $GOVIM_VERSION
git checkout master
6 changes: 5 additions & 1 deletion gopls/integration/govim/run_tests_for_cloudbuild.sh
Original file line number Diff line number Diff line change
@@ -9,8 +9,12 @@
# build step. We do this so that we can capture govim test artifacts regardless
# of the test results.

# See golang.org/issues/38042. Temporarily disable telemetry until event
# exporters are threadsafe.
export GOVIM_GOPLS_FLAGS="-telemetry.disable"

# Substitute the locally built gopls binary for use in govim integration tests.
go test ./cmd/govim -gopls /workspace/gopls/gopls
go test -short ./cmd/govim -gopls /workspace/gopls/gopls

# Stash the error, for use in a later build step.
echo "exit $?" > /workspace/govim_test_result.sh
8 changes: 8 additions & 0 deletions internal/lsp/cmd/serve.go
Original file line number Diff line number Diff line change
@@ -17,6 +17,7 @@ import (
"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/telemetry/event"
"golang.org/x/tools/internal/tool"
)

@@ -34,6 +35,7 @@ type Serve struct {
RemoteListenTimeout time.Duration `flag:"remote.listen.timeout" help:"when used with -remote=auto, the listen.timeout used when auto-starting the remote"`
RemoteDebug string `flag:"remote.debug" help:"when used with -remote=auto, the debug address used when auto-starting the remote"`
RemoteLogfile string `flag:"remote.logfile" help:"when used with -remote=auto, the filename for the remote daemon to log to"`
DisableExport bool `flag:"telemetry.disable" help:"TEMPORARY WORKAROUND: disable telemetry processing entirely. This flag will be removed in the future, once telemetry issues are resolved."`

app *Application
}
@@ -60,6 +62,12 @@ func (s *Serve) Run(ctx context.Context, args ...string) error {
return tool.CommandLineErrorf("server does not take arguments, got %v", args)
}

// Temporary workaround for golang.org/issues/38042: allow disabling
// telemetry export.
if s.DisableExport {
event.SetExporter(nil)
}

di := debug.GetInstance(ctx)
if di != nil {
closeLog, err := di.SetLogFile(s.Logfile)

0 comments on commit 17a19b5

Please sign in to comment.