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 7, 2024 · a580d97 · a580d97
1 parent 22d4212
commit a580d97
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 44 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.SpanExporterDetector(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 {
+	DetectSpanExporter() (sdktrace.SpanExporter, error)
+	DetectMetricExporter() (sdkmetric.Exporter, error)
+}
 
 type detector struct {
 	f        ExporterDetector
@@ -52,17 +55,42 @@ func Register(name string, exp ExporterDetector, priority int) {
 	}
 }
 
+type SpanExporterDetector func() (sdktrace.SpanExporter, error)
+
+func (fn SpanExporterDetector) DetectSpanExporter() (sdktrace.SpanExporter, error) {
+	return fn()
+}
+
+func (fn SpanExporterDetector) DetectMetricExporter() (sdkmetric.Exporter, error) {
+	return nil, nil
+}
+
 func detectExporter() (texp sdktrace.SpanExporter, mexp sdkmetric.Exporter, err error) {
-	if n := os.Getenv("OTEL_TRACES_EXPORTER"); n != "" {
+	texp, err = detectExporterHelper("OTEL_TRACES_EXPORTER", func(d ExporterDetector) (sdktrace.SpanExporter, error) {
+		return d.DetectSpanExporter()
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+
+	mexp, err = detectExporterHelper("OTEL_METRICS_EXPORTER", func(d ExporterDetector) (sdkmetric.Exporter, error) {
+		return d.DetectMetricExporter()
+	})
+	if err != nil {
+		return nil, nil, err
+	}
+	return texp, mexp, nil
+}
+
+func detectExporterHelper[T any](envVar string, fn func(d ExporterDetector) (T, 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()
+		return fn(d.f)
 	}
+
 	arr := make([]detector, 0, len(detectors))
 	for _, d := range detectors {
 		arr = append(arr, d)
@@ -72,23 +100,16 @@ func detectExporter() (texp sdktrace.SpanExporter, mexp sdkmetric.Exporter, err
 	})
 
 	for _, d := range arr {
-		t, m, err := d.f()
+		exp, 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 exp != nil {
+			break
 		}
 	}
-	return texp, mexp, nil
+	return exp, nil
 }
 
 func getExporters() (sdktrace.SpanExporter, sdkmetric.Exporter, error) {
@@ -253,3 +274,20 @@ func (serviceNameDetector) Detect(ctx context.Context) (*resource.Resource, erro
 		},
 	).Detect(ctx)
 }
+
+type noneDetector struct{}
+
+func (n noneDetector) DetectSpanExporter() (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.SpanExporterDetector(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) DetectSpanExporter() (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