Merge pull request #4745 from jsternberg/none-metrics-exporter

detect: fix auto-detection of metric exporters to handle none correctly
moby · Mar 11, 2024 · 517e253 · 517e253
2 parents d557cb7 + 3947d5b
commit 517e253
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