From a1c56757aa42df2baa8ff0c660a0ce2b468f1723 Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Fri, 28 Feb 2020 22:55:50 -0500 Subject: [PATCH] internal/telemetry: remove the concept of a Tagger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Tagger interface allowed for specifying a key in a tag list and having the value be aquired from the context automatically. It was almost never used, and the alternative (using the key to get the value) is not that much more long winded, so it was not holding it's own weight from a complexity persective alone but the performance cost of having to use a list of interface rather than a list of tags was very large. See the benchstat improvements below for the difference it makes to both speed and memory usage, especially in the no exporter case. name old time/op new time/op delta Baseline-8 341ns ± 2% 342ns ± 1% ~ (p=0.139 n=19+18) LoggingNoExporter-8 2.46µs ± 5% 1.97µs ± 3% -19.88% (p=0.000 n=19+20) Logging-8 13.3µs ± 2% 12.8µs ± 2% -3.42% (p=0.000 n=17+20) LoggingStdlib-8 5.39µs ± 6% 5.34µs ± 3% ~ (p=0.692 n=20+19) name old alloc/op new alloc/op delta Baseline-8 80.0B ± 0% 80.0B ± 0% ~ (all equal) LoggingNoExporter-8 728B ± 0% 440B ± 0% -39.56% (p=0.000 n=20+20) Logging-8 2.75kB ± 0% 2.46kB ± 0% -10.53% (p=0.000 n=17+20) LoggingStdlib-8 568B ± 0% 568B ± 0% ~ (all equal) name old allocs/op new allocs/op delta Baseline-8 5.00 ± 0% 5.00 ± 0% ~ (all equal) LoggingNoExporter-8 32.0 ± 0% 23.0 ± 0% -28.12% (p=0.000 n=20+20) Logging-8 88.0 ± 0% 79.0 ± 0% -10.23% (p=0.000 n=20+20) LoggingStdlib-8 28.0 ± 0% 28.0 ± 0% ~ (all equal) Change-Id: Ic203ad0c5de7451348976b999a0d038ac532dc39 Reviewed-on: https://go-review.googlesource.com/c/tools/+/221737 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Robert Findley Reviewed-by: Emmanuel Odeke --- internal/lsp/diagnostics.go | 2 +- internal/telemetry/log/log.go | 9 ++++----- internal/telemetry/tag.go | 7 ------- internal/telemetry/tag/tag.go | 17 ----------------- 4 files changed, 5 insertions(+), 30 deletions(-) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 0319c08d24..de0e23e7f6 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -203,7 +203,7 @@ func (s *Server) publishReports(ctx context.Context, snapshot source.Snapshot, r Version: key.id.Version, }); err != nil { if ctx.Err() == nil { - log.Error(ctx, "publishReports: failed to deliver diagnostic", err, telemetry.File) + log.Error(ctx, "publishReports: failed to deliver diagnostic", err, telemetry.File.Tag(ctx)) } continue } diff --git a/internal/telemetry/log/log.go b/internal/telemetry/log/log.go index b4f2c70a21..0c57e8a24c 100644 --- a/internal/telemetry/log/log.go +++ b/internal/telemetry/log/log.go @@ -12,7 +12,6 @@ import ( "golang.org/x/tools/internal/telemetry" "golang.org/x/tools/internal/telemetry/export" - "golang.org/x/tools/internal/telemetry/tag" ) type Event telemetry.Event @@ -27,18 +26,18 @@ func With(ctx context.Context, tags ...telemetry.Tag) { // Print takes a message and a tag list and combines them into a single tag // list before delivering them to the loggers. -func Print(ctx context.Context, message string, tags ...tag.Tagger) { +func Print(ctx context.Context, message string, tags ...telemetry.Tag) { export.Log(ctx, telemetry.Event{ At: time.Now(), Message: message, - Tags: tag.Tags(ctx, tags...), + Tags: tags, }) } // Error takes a message and a tag list and combines them into a single tag // list before delivering them to the loggers. It captures the error in the // delivered event. -func Error(ctx context.Context, message string, err error, tags ...tag.Tagger) { +func Error(ctx context.Context, message string, err error, tags ...telemetry.Tag) { if err == nil { err = errorString(message) message = "" @@ -47,7 +46,7 @@ func Error(ctx context.Context, message string, err error, tags ...tag.Tagger) { At: time.Now(), Message: message, Error: err, - Tags: tag.Tags(ctx, tags...), + Tags: tags, }) } diff --git a/internal/telemetry/tag.go b/internal/telemetry/tag.go index 54c6102f48..4af028d6dc 100644 --- a/internal/telemetry/tag.go +++ b/internal/telemetry/tag.go @@ -5,7 +5,6 @@ package telemetry import ( - "context" "fmt" ) @@ -26,12 +25,6 @@ func (t Tag) Format(f fmt.State, r rune) { fmt.Fprintf(f, `%v="%v"`, t.Key, t.Value) } -// Tag returns the tag unmodified. -// It makes Key conform to the Tagger interface. -func (t Tag) Tag(ctx context.Context) Tag { - return t -} - // Get will get a single key's value from the list. func (l TagList) Get(k interface{}) interface{} { for _, t := range l { diff --git a/internal/telemetry/tag/tag.go b/internal/telemetry/tag/tag.go index c6192aba95..2c96aaadba 100644 --- a/internal/telemetry/tag/tag.go +++ b/internal/telemetry/tag/tag.go @@ -18,14 +18,6 @@ import ( //TODO: Do we need to do something more efficient than just store tags //TODO: directly on the context? -// Tagger is the interface to something that returns a Tag given a context. -// Both Tag itself and Key support this interface, allowing methods that can -// take either (and other implementations as well) -type Tagger interface { - // Tag returns a Tag potentially using information from the Context. - Tag(context.Context) telemetry.Tag -} - // With is roughly equivalent to context.WithValue except that it also notifies // registered observers. // Unlike WithValue, it takes a list of tags so that you can set many values @@ -48,12 +40,3 @@ func Get(ctx context.Context, keys ...interface{}) telemetry.TagList { } return tags } - -// Tags collects a list of tags for the taggers from the context. -func Tags(ctx context.Context, taggers ...Tagger) telemetry.TagList { - tags := make(telemetry.TagList, len(taggers)) - for i, t := range taggers { - tags[i] = t.Tag(ctx) - } - return tags -}