internal/telemetry: removing the concept of exporter lists

Instead we only have a single exporter, and it must delegate behaviour
to any other exporters it wants to include.
This removes a whole collection of suprises caused by init functions adding
new exporters to a list, as well as generally making things faster, at the small
expense of needing to implement a custom exporter if you want to combine the
features of a few other exporters.

This is essentially the opposite of https://go-review.googlesource.com/c/tools/+/212243
which will now be abandoned in favor of this approach.

Change-Id: Icacb4c1f0f40f99ddd1d82c73d4f25a3486e56ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/220857
Run-TryBot: Ian Cottrell <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Emmanuel Odeke <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: ianthehat

internal/lsp/debug/rpc.go

@@ -90,11 +90,6 @@ type rpcCodeBucket struct {
 	Count int64
 }
 
-func (r *rpcs) StartSpan(ctx context.Context, span *telemetry.Span)  {}
-func (r *rpcs) FinishSpan(ctx context.Context, span *telemetry.Span) {}
-func (r *rpcs) Log(ctx context.Context, event telemetry.Event)       {}
-func (r *rpcs) Flush()                                               {}
-
 func (r *rpcs) Metric(ctx context.Context, data telemetry.MetricData) {
 	for i, group := range data.Groups() {
 		set := &r.Inbound

internal/lsp/debug/serve.go

@@ -27,7 +27,9 @@ import (
 	"sync"
 	"time"
 
+	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/span"
+	"golang.org/x/tools/internal/telemetry"
 	"golang.org/x/tools/internal/telemetry/export"
 	"golang.org/x/tools/internal/telemetry/export/ocagent"
 	"golang.org/x/tools/internal/telemetry/export/prometheus"
@@ -368,7 +370,7 @@ func NewInstance(workdir, agent string) *Instance {
 	i.rpcs = &rpcs{}
 	i.traces = &traces{}
 	i.State = &State{}
-	export.AddExporters(i.ocagent, i.prometheus, i.rpcs, i.traces)
+	export.SetExporter(i)
 	return i
 }
 
@@ -493,6 +495,57 @@ func (i *Instance) writeMemoryDebug(threshold uint64) error {
 	return nil
 }
 
+func (i *Instance) StartSpan(ctx context.Context, spn *telemetry.Span) {
+	if i.ocagent != nil {
+		i.ocagent.StartSpan(ctx, spn)
+	}
+	if i.traces != nil {
+		i.traces.StartSpan(ctx, spn)
+	}
+}
+
+func (i *Instance) FinishSpan(ctx context.Context, spn *telemetry.Span) {
+	if i.ocagent != nil {
+		i.ocagent.FinishSpan(ctx, spn)
+	}
+	if i.traces != nil {
+		i.traces.FinishSpan(ctx, spn)
+	}
+}
+
+//TODO: remove this hack
+// capture stderr at startup because it gets modified in a way that this
+// logger should not respect
+var stderr = os.Stderr
+
+func (i *Instance) Log(ctx context.Context, event telemetry.Event) {
+	if event.Error != nil {
+		fmt.Fprintf(stderr, "%v\n", event)
+	}
+	protocol.LogEvent(ctx, event)
+	if i.ocagent != nil {
+		i.ocagent.Log(ctx, event)
+	}
+}
+
+func (i *Instance) Metric(ctx context.Context, data telemetry.MetricData) {
+	if i.ocagent != nil {
+		i.ocagent.Metric(ctx, data)
+	}
+	if i.traces != nil {
+		i.prometheus.Metric(ctx, data)
+	}
+	if i.rpcs != nil {
+		i.rpcs.Metric(ctx, data)
+	}
+}
+
+func (i *Instance) Flush() {
+	if i.ocagent != nil {
+		i.ocagent.Flush()
+	}
+}
+
 type dataFunc func(*http.Request) interface{}
 
 func render(tmpl *template.Template, fun dataFunc) func(http.ResponseWriter, *http.Request) {

internal/lsp/debug/trace.go

@@ -130,12 +130,6 @@ func (t *traces) FinishSpan(ctx context.Context, span *telemetry.Span) {
 	}
 }
 
-func (t *traces) Log(ctx context.Context, event telemetry.Event) {}
-
-func (t *traces) Metric(ctx context.Context, data telemetry.MetricData) {}
-
-func (t *traces) Flush() {}
-
 func (t *traces) getData(req *http.Request) interface{} {
 	if len(t.sets) == 0 {
 		return nil

internal/lsp/protocol/context.go

@@ -5,14 +5,9 @@ import (
 	"fmt"
 
 	"golang.org/x/tools/internal/telemetry"
-	"golang.org/x/tools/internal/telemetry/export"
 	"golang.org/x/tools/internal/xcontext"
 )
 
-func init() {
-	export.AddExporters(logExporter{})
-}
-
 type contextKey int
 
 const (
@@ -23,16 +18,7 @@ func WithClient(ctx context.Context, client Client) context.Context {
 	return context.WithValue(ctx, clientKey, client)
 }
 
-// logExporter sends the log event back to the client if there is one stored on the
-// context.
-type logExporter struct{}
-
-func (logExporter) StartSpan(context.Context, *telemetry.Span)   {}
-func (logExporter) FinishSpan(context.Context, *telemetry.Span)  {}
-func (logExporter) Metric(context.Context, telemetry.MetricData) {}
-func (logExporter) Flush()                                       {}
-
-func (logExporter) Log(ctx context.Context, event telemetry.Event) {
+func LogEvent(ctx context.Context, event telemetry.Event) {
 	client, ok := ctx.Value(clientKey).(Client)
 	if !ok {
 		return

internal/telemetry/export/export.go

@@ -41,29 +41,32 @@ func SetExporter(e Exporter) {
 	exporter = e
 }
 
-func AddExporters(e ...Exporter) {
-	exporterMu.Lock()
-	defer exporterMu.Unlock()
-	exporter = Multi(append([]Exporter{exporter}, e...)...)
-}
-
 func StartSpan(ctx context.Context, span *telemetry.Span, at time.Time) {
 	exporterMu.Lock()
 	defer exporterMu.Unlock()
+	if exporter == nil {
+		return
+	}
 	span.Start = at
 	exporter.StartSpan(ctx, span)
 }
 
 func FinishSpan(ctx context.Context, span *telemetry.Span, at time.Time) {
 	exporterMu.Lock()
 	defer exporterMu.Unlock()
+	if exporter == nil {
+		return
+	}
 	span.Finish = at
 	exporter.FinishSpan(ctx, span)
 }
 
 func Tag(ctx context.Context, at time.Time, tags telemetry.TagList) {
 	exporterMu.Lock()
 	defer exporterMu.Unlock()
+	if exporter == nil {
+		return
+	}
 	// If context has a span we need to add the tags to it
 	span := telemetry.GetSpan(ctx)
 	if span == nil {
@@ -84,6 +87,9 @@ func Tag(ctx context.Context, at time.Time, tags telemetry.TagList) {
 func Log(ctx context.Context, event telemetry.Event) {
 	exporterMu.Lock()
 	defer exporterMu.Unlock()
+	if exporter == nil {
+		return
+	}
 	// If context has a span we need to add the event to it
 	span := telemetry.GetSpan(ctx)
 	if span != nil {
@@ -96,11 +102,17 @@ func Log(ctx context.Context, event telemetry.Event) {
 func Metric(ctx context.Context, data telemetry.MetricData) {
 	exporterMu.Lock()
 	defer exporterMu.Unlock()
+	if exporter == nil {
+		return
+	}
 	exporter.Metric(ctx, data)
 }
 
 func Flush() {
 	exporterMu.Lock()
 	defer exporterMu.Unlock()
+	if exporter == nil {
+		return
+	}
 	exporter.Flush()
 }

internal/telemetry/export/multi.go

@@ -79,7 +79,7 @@ func main() {
 		Rate:    5 * time.Second,
 		Client:  &http.Client{},
 	})
-	export.AddExporters(exporter)
+	export.SetExporter(exporter)
 
 	ctx := context.TODO()
 	mLatency := stats.Float64("latency", "the latency in milliseconds", "ms")

internal/telemetry/export/null.go

@@ -25,11 +25,6 @@ type Exporter struct {
 	metrics []telemetry.MetricData
 }
 
-func (e *Exporter) StartSpan(ctx context.Context, span *telemetry.Span)  {}
-func (e *Exporter) FinishSpan(ctx context.Context, span *telemetry.Span) {}
-func (e *Exporter) Log(ctx context.Context, event telemetry.Event)       {}
-func (e *Exporter) Flush()                                               {}
-
 func (e *Exporter) Metric(ctx context.Context, data telemetry.MetricData) {
 	e.mu.Lock()
 	defer e.mu.Unlock()

internal/telemetry/export/ocagent/README.md

@@ -91,7 +91,7 @@ func BenchmarkBaseline(b *testing.B) {
 
 func BenchmarkLoggingNoExporter(b *testing.B) {
 	ctx := context.Background()
-	export.SetExporter(export.Null())
+	export.SetExporter(nil)
 	b.ReportAllocs()
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {