detect: fix auto-detection of metric exporters to handle none correctly

The original design for adding metric exporters accidentally relied on the traces exporter variable for enabling or disabling the metrics exporter. It's normal for metrics to get auto-enabled with the same environment variables that traces use, but it should also respond to `none` for disabling. Signed-off-by: Jonathan A. Sternberg <[email protected]>
moby · Mar 11, 2024 · 3947d5b · 3947d5b
1 parent 22d4212
commit 3947d5b
Show file tree

Hide file tree

Showing 4 changed files with 77 additions and 46 deletions.
diff --git a/util/tracing/detect/delegated/delegated.go b/util/tracing/detect/delegated/delegated.go
@@ -5,7 +5,6 @@ import (
 	"sync"
 
 	"github.com/moby/buildkit/util/tracing/detect"
-	sdkmetric "go.opentelemetry.io/otel/sdk/metric"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 )
 
@@ -14,9 +13,9 @@ const maxBuffer = 256
 var exp = &Exporter{}
 
 func init() {
-	detect.Register("delegated", func() (sdktrace.SpanExporter, sdkmetric.Exporter, error) {
-		return exp, nil, nil
-	}, 100)
+	detect.Register("delegated", detect.TraceExporterDetector(func() (sdktrace.SpanExporter, error) {
+		return exp, nil
+	}), 100)
 }
 
 type Exporter struct {

diff --git a/util/tracing/detect/detect.go b/util/tracing/detect/detect.go
@@ -21,7 +21,10 @@ import (
 	"go.opentelemetry.io/otel/trace/noop"
 )
 
-type ExporterDetector func() (sdktrace.SpanExporter, sdkmetric.Exporter, error)
+type ExporterDetector interface {
+	DetectTraceExporter() (sdktrace.SpanExporter, error)
+	DetectMetricExporter() (sdkmetric.Exporter, error)
+}
 
 type detector struct {
 	f        ExporterDetector
@@ -52,17 +55,45 @@ func Register(name string, exp ExporterDetector, priority int) {
 	}
 }
 
-func detectExporter() (texp sdktrace.SpanExporter, mexp sdkmetric.Exporter, err error) {
-	if n := os.Getenv("OTEL_TRACES_EXPORTER"); n != "" {
+type TraceExporterDetector func() (sdktrace.SpanExporter, error)
+
+func (fn TraceExporterDetector) DetectTraceExporter() (sdktrace.SpanExporter, error) {
+	return fn()
+}
+
+func (fn TraceExporterDetector) DetectMetricExporter() (sdkmetric.Exporter, error) {
+	return nil, nil
+}
+
+func detectExporters() (texp sdktrace.SpanExporter, mexp sdkmetric.Exporter, err error) {
+	texp, err = detectExporter("OTEL_TRACES_EXPORTER", func(d ExporterDetector) (sdktrace.SpanExporter, bool, error) {
+		exp, err := d.DetectTraceExporter()
+		return exp, exp != nil, err
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+
+	mexp, err = detectExporter("OTEL_METRICS_EXPORTER", func(d ExporterDetector) (sdkmetric.Exporter, bool, error) {
+		exp, err := d.DetectMetricExporter()
+		return exp, exp != nil, err
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+	return texp, mexp, nil
+}
+
+func detectExporter[T any](envVar string, fn func(d ExporterDetector) (T, bool, error)) (exp T, err error) {
+	if n := os.Getenv(envVar); n != "" {
 		d, ok := detectors[n]
 		if !ok {
-			if n == "none" {
-				return nil, nil, nil
-			}
-			return nil, nil, errors.Errorf("unsupported opentelemetry tracer %v", n)
+			return exp, errors.Errorf("unsupported opentelemetry exporter %v", n)
 		}
-		return d.f()
+		exp, _, err = fn(d.f)
+		return exp, err
 	}
+
 	arr := make([]detector, 0, len(detectors))
 	for _, d := range detectors {
 		arr = append(arr, d)
@@ -71,28 +102,22 @@ func detectExporter() (texp sdktrace.SpanExporter, mexp sdkmetric.Exporter, err
 		return arr[i].priority < arr[j].priority
 	})
 
+	var ok bool
 	for _, d := range arr {
-		t, m, err := d.f()
+		exp, ok, err = fn(d.f)
 		if err != nil {
-			return nil, nil, err
-		}
-		if texp == nil {
-			texp = t
-		}
-		if mexp == nil {
-			mexp = m
+			return exp, err
 		}
 
-		// Found a candidate for both exporters so just return now.
-		if texp != nil && mexp != nil {
-			return texp, mexp, nil
+		if ok {
+			break
 		}
 	}
-	return texp, mexp, nil
+	return exp, nil
 }
 
 func getExporters() (sdktrace.SpanExporter, sdkmetric.Exporter, error) {
-	texp, mexp, err := detectExporter()
+	texp, mexp, err := detectExporters()
 	if err != nil {
 		return nil, nil, err
 	}
@@ -253,3 +278,20 @@ func (serviceNameDetector) Detect(ctx context.Context) (*resource.Resource, erro
 		},
 	).Detect(ctx)
 }
+
+type noneDetector struct{}
+
+func (n noneDetector) DetectTraceExporter() (sdktrace.SpanExporter, error) {
+	return nil, nil
+}
+
+func (n noneDetector) DetectMetricExporter() (sdkmetric.Exporter, error) {
+	return nil, nil
+}
+
+func init() {
+	// Register a none detector. This will never be chosen if there's another suitable
+	// exporter that can be detected, but exists to allow telemetry to be explicitly
+	// disabled.
+	Register("none", noneDetector{}, 1000)
+}
diff --git a/util/tracing/detect/jaeger/jaeger.go b/util/tracing/detect/jaeger/jaeger.go
@@ -10,18 +10,17 @@ import (
 	"github.com/moby/buildkit/util/tracing/detect"
 	//nolint:staticcheck // Jaeger still supported for compatibility
 	"go.opentelemetry.io/otel/exporters/jaeger"
-	sdkmetric "go.opentelemetry.io/otel/sdk/metric"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 )
 
 func init() {
-	detect.Register("jaeger", jaegerExporter, 11)
+	detect.Register("jaeger", detect.TraceExporterDetector(jaegerExporter), 11)
 }
 
-func jaegerExporter() (sdktrace.SpanExporter, sdkmetric.Exporter, error) {
+func jaegerExporter() (sdktrace.SpanExporter, error) {
 	set := os.Getenv("OTEL_TRACES_EXPORTER") == "jaeger" || os.Getenv("JAEGER_TRACE") != "" || os.Getenv("OTEL_EXPORTER_JAEGER_AGENT_HOST") != "" || os.Getenv("OTEL_EXPORTER_JAEGER_ENDPOINT") != ""
 	if !set {
-		return nil, nil, nil
+		return nil, nil
 	}
 
 	endpoint := envOr("OTEL_EXPORTER_JAEGER_ENDPOINT", "http://localhost:14250")
@@ -37,7 +36,7 @@ func jaegerExporter() (sdktrace.SpanExporter, sdkmetric.Exporter, error) {
 		} else {
 			h, p, err := net.SplitHostPort(v)
 			if err != nil {
-				return nil, nil, err
+				return nil, err
 			}
 			host = h
 			port = p
@@ -54,12 +53,12 @@ func jaegerExporter() (sdktrace.SpanExporter, sdkmetric.Exporter, error) {
 
 	exp, err := jaeger.New(epo)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	return &threadSafeExporterWrapper{
 		exporter: exp,
-	}, nil, nil
+	}, nil
 }
 
 func envOr(key, defaultValue string) string {

diff --git a/util/tracing/detect/otlp.go b/util/tracing/detect/otlp.go
@@ -15,24 +15,15 @@ import (
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 )
 
+var otlpExporter = otlpExporterDetector{}
+
 func init() {
 	Register("otlp", otlpExporter, 10)
 }
 
-func otlpExporter() (sdktrace.SpanExporter, sdkmetric.Exporter, error) {
-	texp, err := otlpSpanExporter()
-	if err != nil {
-		return nil, nil, err
-	}
-
-	mexp, err := otlpMetricExporter()
-	if err != nil {
-		return nil, nil, err
-	}
-	return texp, mexp, nil
-}
+type otlpExporterDetector struct{}
 
-func otlpSpanExporter() (sdktrace.SpanExporter, error) {
+func (otlpExporterDetector) DetectTraceExporter() (sdktrace.SpanExporter, error) {
 	set := os.Getenv("OTEL_TRACES_EXPORTER") == "otlp" || os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" || os.Getenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") != ""
 	if !set {
 		return nil, nil
@@ -61,7 +52,7 @@ func otlpSpanExporter() (sdktrace.SpanExporter, error) {
 	return otlptrace.New(context.Background(), c)
 }
 
-func otlpMetricExporter() (sdkmetric.Exporter, error) {
+func (otlpExporterDetector) DetectMetricExporter() (sdkmetric.Exporter, error) {
 	set := os.Getenv("OTEL_METRICS_EXPORTER") == "otlp" || os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" || os.Getenv("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT") != ""
 	if !set {
 		return nil, nil