From 32f14692fca29d9b7ec4e1bbb54751ad0c5bee0f Mon Sep 17 00:00:00 2001
From: Ian Cottrell <iancottrell@google.com>
Date: Wed, 4 Mar 2020 16:41:57 -0500
Subject: [PATCH] internal/lsp: use standardised events for tagging

This means that tags also become cheap if there is no exporter and cleans up the
mess with how spans, tags and logs were related.
Also fixes the currently broken metrics that relied on the span tags.

Change-Id: I8e56b6218a60fd31a1f6c8d329dbb2cab1b9254d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/222065
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
---
 internal/lsp/debug/serve.go         |  1 +
 internal/telemetry/event.go         |  1 +
 internal/telemetry/export/export.go | 23 -----------------------
 internal/telemetry/export/tag.go    | 25 +++++++++++++++++++++++++
 internal/telemetry/export/trace.go  |  7 ++++++-
 internal/telemetry/tag/tag.go       | 20 ++++++--------------
 6 files changed, 39 insertions(+), 38 deletions(-)
 create mode 100644 internal/telemetry/export/tag.go

diff --git a/internal/lsp/debug/serve.go b/internal/lsp/debug/serve.go
index 4e48f128cf..828769aa5e 100644
--- a/internal/lsp/debug/serve.go
+++ b/internal/lsp/debug/serve.go
@@ -554,6 +554,7 @@ func (e *exporter) ProcessEvent(ctx context.Context, event telemetry.Event) cont
 	if i == nil {
 		return ctx
 	}
+	ctx = export.Tag(ctx, event)
 	if i.ocagent != nil {
 		ctx = i.ocagent.ProcessEvent(ctx, event)
 	}
diff --git a/internal/telemetry/event.go b/internal/telemetry/event.go
index 3eaf094c74..df202665fa 100644
--- a/internal/telemetry/event.go
+++ b/internal/telemetry/event.go
@@ -15,6 +15,7 @@ const (
 	EventLog = EventType(iota)
 	EventStartSpan
 	EventEndSpan
+	EventTag
 )
 
 type Event struct {
diff --git a/internal/telemetry/export/export.go b/internal/telemetry/export/export.go
index a1f6dfaa7b..ae14c4e67d 100644
--- a/internal/telemetry/export/export.go
+++ b/internal/telemetry/export/export.go
@@ -11,7 +11,6 @@ import (
 	"context"
 	"os"
 	"sync/atomic"
-	"time"
 	"unsafe"
 
 	"golang.org/x/tools/internal/telemetry"
@@ -42,28 +41,6 @@ func SetExporter(e Exporter) {
 	atomic.StorePointer(&exporter, p)
 }
 
-func Tag(ctx context.Context, at time.Time, tags telemetry.TagList) {
-	exporterPtr := (*Exporter)(atomic.LoadPointer(&exporter))
-	if exporterPtr == nil {
-		return
-	}
-	// If context has a span we need to add the tags to it
-	span := GetSpan(ctx)
-	if span == nil {
-		return
-	}
-	if span.Start.IsZero() {
-		// span still being created, tag it directly
-		span.Tags = append(span.Tags, tags...)
-		return
-	}
-	// span in progress, add an event to the span
-	span.Events = append(span.Events, telemetry.Event{
-		At:   at,
-		Tags: tags,
-	})
-}
-
 func ProcessEvent(ctx context.Context, event telemetry.Event) context.Context {
 	exporterPtr := (*Exporter)(atomic.LoadPointer(&exporter))
 	if exporterPtr == nil {
diff --git a/internal/telemetry/export/tag.go b/internal/telemetry/export/tag.go
new file mode 100644
index 0000000000..24825a3454
--- /dev/null
+++ b/internal/telemetry/export/tag.go
@@ -0,0 +1,25 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package export
+
+import (
+	"context"
+
+	"golang.org/x/tools/internal/telemetry"
+)
+
+// Tag returns a context updated with tag values from the event.
+// It ignores events that are not or type EventTag or EventStartSpan.
+func Tag(ctx context.Context, event telemetry.Event) context.Context {
+	//TODO: Do we need to do something more efficient than just store tags
+	//TODO: directly on the context?
+	switch event.Type {
+	case telemetry.EventTag, telemetry.EventStartSpan:
+		for _, t := range event.Tags {
+			ctx = context.WithValue(ctx, t.Key, t.Value)
+		}
+	}
+	return ctx
+}
diff --git a/internal/telemetry/export/trace.go b/internal/telemetry/export/trace.go
index f45e049594..bf28285f20 100644
--- a/internal/telemetry/export/trace.go
+++ b/internal/telemetry/export/trace.go
@@ -41,9 +41,14 @@ func GetSpan(ctx context.Context) *Span {
 	return v.(*Span)
 }
 
+// ContextSpan is an exporter that maintains hierarchical span structure in the
+// context.
+// It creates new spans on EventStartSpan, adds events to the current span on
+// EventLog or EventTag, and closes the span on EventEndSpan.
+// The span structure can then be used by other exporters.
 func ContextSpan(ctx context.Context, event telemetry.Event) context.Context {
 	switch event.Type {
-	case telemetry.EventLog:
+	case telemetry.EventLog, telemetry.EventTag:
 		if span := GetSpan(ctx); span != nil {
 			span.Events = append(span.Events, event)
 		}
diff --git a/internal/telemetry/tag/tag.go b/internal/telemetry/tag/tag.go
index 2c96aaadba..48ca7ccaab 100644
--- a/internal/telemetry/tag/tag.go
+++ b/internal/telemetry/tag/tag.go
@@ -15,21 +15,13 @@ import (
 	"golang.org/x/tools/internal/telemetry/export"
 )
 
-//TODO: Do we need to do something more efficient than just store tags
-//TODO: directly on the context?
-
-// 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
-// at once if needed. Each call to With results in one invocation of each
-// observer.
+// With delivers the tag list to the telemetry exporter.
 func With(ctx context.Context, tags ...telemetry.Tag) context.Context {
-	at := time.Now()
-	for _, t := range tags {
-		ctx = context.WithValue(ctx, t.Key, t.Value)
-	}
-	export.Tag(ctx, at, tags)
-	return ctx
+	return export.ProcessEvent(ctx, telemetry.Event{
+		Type: telemetry.EventTag,
+		At:   time.Now(),
+		Tags: tags,
+	})
 }
 
 // Get collects a set of values from the context and returns them as a tag list.