From d7fc2cf50e39d8ef74453bed096595c43fe4aabf Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Tue, 10 Mar 2020 23:25:10 -0400 Subject: [PATCH] internal/telemetry: delete the event.TagOf method It encourages poor performing log lines, and also reduces the readability of those lines. Also delete the Key.With method which was unused, and should never be used instead of the event.Label function anyway. Change-Id: I9b55102864ee49a7d03e60af022a2002170c0fb1 Reviewed-on: https://go-review.googlesource.com/c/tools/+/222851 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Emmanuel Odeke --- internal/lsp/cache/analysis.go | 2 +- internal/lsp/cache/load.go | 4 ++-- internal/lsp/cache/view.go | 2 +- internal/lsp/completion.go | 3 ++- internal/lsp/debug/serve.go | 3 ++- internal/lsp/debug/tag/tag.go | 8 ++++++++ internal/lsp/signature_help.go | 3 ++- internal/lsp/source/completion_format.go | 2 +- internal/lsp/source/options.go | 4 ++-- internal/telemetry/event/key.go | 6 ------ 10 files changed, 21 insertions(+), 16 deletions(-) diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index a517223127..cd1a7186cc 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -343,7 +343,7 @@ func runAnalysis(ctx context.Context, fset *token.FileSet, analyzer *analysis.An for _, diag := range diagnostics { srcErr, err := sourceError(ctx, fset, pkg, diag) if err != nil { - event.Error(ctx, "unable to compute analysis error position", err, event.TagOf("category", diag.Category), tag.Package.Of(pkg.ID())) + event.Error(ctx, "unable to compute analysis error position", err, tag.Category.Of(diag.Category), tag.Package.Of(pkg.ID())) continue } data.diagnostics = append(data.diagnostics, srcErr) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index d7b5b582c2..a778c8841b 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -90,13 +90,13 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { return ctx.Err() } - event.Print(ctx, "go/packages.Load", event.TagOf("snapshot", s.ID()), event.TagOf("query", query), event.TagOf("packages", len(pkgs))) + event.Print(ctx, "go/packages.Load", tag.Snapshot.Of(s.ID()), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs))) if len(pkgs) == 0 { return err } for _, pkg := range pkgs { if !containsDir || s.view.Options().VerboseOutput { - event.Print(ctx, "go/packages.Load", event.TagOf("snapshot", s.ID()), event.TagOf("package", pkg.PkgPath), event.TagOf("files", pkg.CompiledGoFiles)) + event.Print(ctx, "go/packages.Load", tag.Snapshot.Of(s.ID()), tag.PackagePath.Of(pkg.PkgPath), tag.Files.Of(pkg.CompiledGoFiles)) } // Ignore packages with no sources, since we will never be able to // correctly invalidate that metadata. diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 933ec406cc..2feb4c0bc2 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -321,7 +321,7 @@ func (v *view) refreshProcessEnv() { // We don't have a context handy to use for logging, so use the stdlib for now. event.Print(v.baseCtx, "background imports cache refresh starting") err := imports.PrimeCache(context.Background(), env) - event.Print(v.baseCtx, fmt.Sprintf("background refresh finished after %v", time.Since(start)), event.TagOf("Error", err)) + event.Print(v.baseCtx, fmt.Sprintf("background refresh finished after %v", time.Since(start)), event.Err.Of(err)) v.importsMu.Lock() v.cacheRefreshDuration = time.Since(start) diff --git a/internal/lsp/completion.go b/internal/lsp/completion.go index e9331dd663..0be6e369b4 100644 --- a/internal/lsp/completion.go +++ b/internal/lsp/completion.go @@ -9,6 +9,7 @@ import ( "fmt" "strings" + "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/telemetry/event" @@ -29,7 +30,7 @@ func (s *Server) completion(ctx context.Context, params *protocol.CompletionPara } if err != nil { - event.Print(ctx, "no completions found", event.TagOf("At", params.Position), event.TagOf("Failure", err)) + event.Print(ctx, "no completions found", tag.Position.Of(params.Position), event.Err.Of("Failure")) } if candidates == nil { return &protocol.CompletionList{ diff --git a/internal/lsp/debug/serve.go b/internal/lsp/debug/serve.go index 7f8e87aad2..315eb4f9c2 100644 --- a/internal/lsp/debug/serve.go +++ b/internal/lsp/debug/serve.go @@ -27,6 +27,7 @@ import ( "sync" "time" + "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/telemetry/event" @@ -462,7 +463,7 @@ func (i *Instance) Serve(ctx context.Context) error { if strings.HasSuffix(i.DebugAddress, ":0") { log.Printf("debug server listening on port %d", port) } - event.Print(ctx, "Debug serving", event.TagOf("Port", port)) + event.Print(ctx, "Debug serving", tag.Port.Of(port)) go func() { mux := http.NewServeMux() mux.HandleFunc("/", render(mainTmpl, func(*http.Request) interface{} { return i })) diff --git a/internal/lsp/debug/tag/tag.go b/internal/lsp/debug/tag/tag.go index d4e9219de4..389fd90841 100644 --- a/internal/lsp/debug/tag/tag.go +++ b/internal/lsp/debug/tag/tag.go @@ -26,6 +26,14 @@ var ( Query = &event.Key{Name: "query"} Snapshot = &event.Key{Name: "snapshot"} Operation = &event.Key{Name: "operation"} + + Position = &event.Key{Name: "position"} + Category = &event.Key{Name: "category"} + PackageCount = &event.Key{Name: "packages"} + Files = &event.Key{Name: "files"} + Port = &event.Key{Name: "port"} + Type = &event.Key{Name: "type"} + HoverKind = &event.Key{Name: "hoverkind"} ) var ( diff --git a/internal/lsp/signature_help.go b/internal/lsp/signature_help.go index e4bb09483b..8bf43f6941 100644 --- a/internal/lsp/signature_help.go +++ b/internal/lsp/signature_help.go @@ -7,6 +7,7 @@ package lsp import ( "context" + "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/telemetry/event" @@ -19,7 +20,7 @@ func (s *Server) signatureHelp(ctx context.Context, params *protocol.SignatureHe } info, activeParameter, err := source.SignatureHelp(ctx, snapshot, fh, params.Position) if err != nil { - event.Print(ctx, "no signature help", event.TagOf("At", params.Position), event.TagOf("Failure", err)) + event.Print(ctx, "no signature help", tag.Position.Of(params.Position), event.Err.Of(err)) return nil, nil } return &protocol.SignatureHelp{ diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index 5144ba561e..00b5ac3530 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -293,7 +293,7 @@ func formatFieldList(ctx context.Context, v View, list *ast.FieldList) ([]string cfg := printer.Config{Mode: printer.UseSpaces | printer.TabIndent, Tabwidth: 4} b := &bytes.Buffer{} if err := cfg.Fprint(b, v.Session().Cache().FileSet(), p.Type); err != nil { - event.Error(ctx, "unable to print type", nil, event.TagOf("Type", p.Type)) + event.Error(ctx, "unable to print type", nil, tag.Type.Of(p.Type)) continue } typ := replacer.Replace(b.String()) diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index 5c1eb17d84..72c3729af3 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -37,10 +37,10 @@ import ( "golang.org/x/tools/go/analysis/passes/unreachable" "golang.org/x/tools/go/analysis/passes/unsafeptr" "golang.org/x/tools/go/analysis/passes/unusedresult" + "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/diff" "golang.org/x/tools/internal/lsp/diff/myers" "golang.org/x/tools/internal/lsp/protocol" - "golang.org/x/tools/internal/telemetry/event" errors "golang.org/x/xerrors" ) @@ -354,7 +354,7 @@ func (o *Options) set(name string, value interface{}) OptionResult { case "Structured": o.HoverKind = Structured default: - result.errorf("Unsupported hover kind", event.TagOf("HoverKind", hoverKind)) + result.errorf("Unsupported hover kind", tag.HoverKind.Of(hoverKind)) } case "linkTarget": diff --git a/internal/telemetry/event/key.go b/internal/telemetry/event/key.go index d24fff684a..bf9f50d37f 100644 --- a/internal/telemetry/event/key.go +++ b/internal/telemetry/event/key.go @@ -17,12 +17,6 @@ type Key struct { Description string } -// TagOf returns a Tag for a key and value. -// This is a trivial helper that makes common logging easier to read. -func TagOf(name string, value interface{}) Tag { - return Tag{Key: &Key{Name: name}, Value: value} -} - // Of creates a new Tag with this key and the supplied value. // You can use this when building a tag list. func (k *Key) Of(v interface{}) Tag {