Remove the WithSDKOptions from the Jaeger exporter (#1825)

* Remove the WithSDKOptions from the Jaeger exporter

* Lint

* Setup SDK in Jaeger example

CHANGELOG.md

@@ -96,6 +96,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   The information that could be configured in the `Process` struct should be configured in a `Resource` instead. (#1776, #1804)
 - Remove the `WithDisabled` option from the Jaeger exporter.
   To disable the exporter unregister it from the `TracerProvider` or use a no-operation `TracerProvider`. (#1806)
+- Removed the Jaeger exporter `WithSDKOptions` `Option`.
+  This option was used to set SDK options for the exporter creation convenience functions.
+  These functions are provided as a way to easily setup or install the exporter with what are deemed reasonable SDK settings for common use cases.
+  If the SDK needs to be configured differently, the `NewRawExporter` function and direct setup of the SDK with the desired settings should be used. (#1825)
 
 ## [0.19.0] - 2021-03-18
 

example/jaeger/main.go

@@ -19,41 +19,71 @@ package main
 import (
 	"context"
 	"log"
+	"time"
 
 	"go.opentelemetry.io/otel"
-	"go.opentelemetry.io/otel/sdk/resource"
-	"go.opentelemetry.io/otel/semconv"
 
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/exporters/trace/jaeger"
-	sdktrace "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/sdk/resource"
+	tracesdk "go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/semconv"
 )
 
-func main() {
-	// Create and install Jaeger export pipeline as the global.
-	tp, err := jaeger.InstallNewPipeline(
-		jaeger.WithCollectorEndpoint("http://localhost:14268/api/traces"),
-		jaeger.WithSDKOptions(
-			sdktrace.WithSampler(sdktrace.AlwaysSample()),
-			sdktrace.WithResource(resource.NewWithAttributes(
-				semconv.ServiceNameKey.String("trace-demo"),
-				attribute.String("exporter", "jaeger"),
-				attribute.Float64("float", 312.23),
-			)),
-		),
+const (
+	service     = "trace-demo"
+	environment = "production"
+	id          = 1
+)
+
+// tracerProvider returns an OpenTelemetry TracerProvider configured to use
+// the Jaeger exporter that will send spans to the provided url. The returned
+// TracerProvider will also use a Resource configured with all the information
+// about the application.
+func tracerProvider(url string) (*tracesdk.TracerProvider, error) {
+	// Create the Jaeger exporter
+	exp, err := jaeger.NewRawExporter(jaeger.WithCollectorEndpoint(url))
+	if err != nil {
+		return nil, err
+	}
+	tp := tracesdk.NewTracerProvider(
+		// Always be sure to batch in production.
+		tracesdk.WithBatcher(exp),
+		// Record information about this application in an Resource.
+		tracesdk.WithResource(resource.NewWithAttributes(
+			semconv.ServiceNameKey.String(service),
+			attribute.String("environment", environment),
+			attribute.Int64("ID", id),
+		)),
 	)
+	return tp, nil
+}
+
+func main() {
+	tp, err := tracerProvider("http://localhost:14268/api/traces")
 	if err != nil {
 		log.Fatal(err)
 	}
-	defer func() {
-		if err := tp.Shutdown(context.Background()); err != nil {
+
+	// Register our TracerProvider as the global so any imported
+	// instrumentation in the future will default to using it.
+	otel.SetTracerProvider(tp)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	// Cleanly shutdown and flush telemetry when the application exits.
+	defer func(ctx context.Context) {
+		// Do not make the application hang when it is shutdown.
+		ctx, cancel = context.WithTimeout(ctx, time.Second*5)
+		defer cancel()
+		if err := tp.Shutdown(ctx); err != nil {
 			log.Fatal(err)
 		}
-	}()
+	}(ctx)
 
 	tr := tp.Tracer("component-main")
 
-	ctx := context.Background()
 	ctx, span := tr.Start(ctx, "foo")
 	defer span.End()
 

exporters/trace/jaeger/jaeger.go

@@ -53,9 +53,6 @@ type options struct {
 
 	// BatchMaxCount defines the maximum number of spans sent in one batch
 	BatchMaxCount int
-
-	// TracerProviderOptions defines the options for tracer provider of sdk.
-	TracerProviderOptions []sdktrace.TracerProviderOption
 }
 
 // WithBufferMaxCount defines the total number of traces that can be buffered in memory
@@ -72,13 +69,6 @@ func WithBatchMaxCount(batchMaxCount int) Option {
 	}
 }
 
-// WithSDKOptions configures options for tracer provider of sdk.
-func WithSDKOptions(opts ...sdktrace.TracerProviderOption) Option {
-	return func(o *options) {
-		o.TracerProviderOptions = opts
-	}
-}
-
 // NewRawExporter returns an OTel Exporter implementation that exports the
 // collected spans to Jaeger.
 func NewRawExporter(endpointOption EndpointOption, opts ...Option) (*Exporter, error) {
@@ -133,18 +123,17 @@ func NewRawExporter(endpointOption EndpointOption, opts ...Option) (*Exporter, e
 // NewExportPipeline sets up a complete export pipeline
 // with the recommended setup for trace provider
 func NewExportPipeline(endpointOption EndpointOption, opts ...Option) (*sdktrace.TracerProvider, error) {
-	o := options{}
-	for _, opt := range opts {
-		opt(&o)
-	}
-
 	exporter, err := NewRawExporter(endpointOption, opts...)
 	if err != nil {
 		return nil, err
 	}
 
-	pOpts := append(o.TracerProviderOptions, sdktrace.WithSyncer(exporter))
-	tp := sdktrace.NewTracerProvider(pOpts...)
+	// TODO (MrAlias): The recommended default setup needs to register the
+	// exporter as a batcher, not a syncer. This will conflict with batching
+	// of the bundler currently, but when the bundler is removed it needs to
+	// be updated.
+	// https://github.com/open-telemetry/opentelemetry-go/issues/1799
+	tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter))
 	return tp, nil
 }
 

exporters/trace/jaeger/jaeger_test.go

@@ -17,6 +17,7 @@ package jaeger
 import (
 	"context"
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"os"
 	"sort"
@@ -54,25 +55,35 @@ func TestInstallNewPipeline(t *testing.T) {
 	assert.IsType(t, tp, otel.GetTracerProvider())
 }
 
-func TestNewRawExporterOptions(t *testing.T) {
-	testCases := []struct {
-		name     string
-		endpoint EndpointOption
+func TestNewExportPipelinePassthroughError(t *testing.T) {
+	for _, testcase := range []struct {
+		name    string
+		failing bool
+		epo     EndpointOption
 	}{
 		{
-			name:     "default exporter with collector endpoint",
-			endpoint: WithCollectorEndpoint(collectorEndpoint),
+			name:    "failing underlying NewRawExporter",
+			failing: true,
+			epo: func() (batchUploader, error) {
+				return nil, errors.New("error")
+			},
 		},
 		{
-			name:     "default exporter with agent endpoint",
-			endpoint: WithAgentEndpoint(),
+			name: "with default agent endpoint",
+			epo:  WithAgentEndpoint(),
 		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			_, err := NewRawExporter(tc.endpoint)
-			assert.NoError(t, err)
+		{
+			name: "with collector endpoint",
+			epo:  WithCollectorEndpoint(collectorEndpoint),
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			_, err := NewExportPipeline(testcase.epo)
+			if testcase.failing {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
 		})
 	}
 }
@@ -202,17 +213,18 @@ func TestExporterExportSpan(t *testing.T) {
 	)
 
 	testCollector := &testCollectorEndpoint{}
-	tp, err := NewExportPipeline(
-		withTestCollectorEndpointInjected(testCollector),
-		WithSDKOptions(
-			sdktrace.WithResource(resource.NewWithAttributes(
-				semconv.ServiceNameKey.String(serviceName),
-				attribute.String(tagKey, tagVal),
-			)),
-		),
-	)
+	exp, err := NewRawExporter(withTestCollectorEndpointInjected(testCollector))
 	require.NoError(t, err)
+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithSyncer(exp),
+		sdktrace.WithResource(resource.NewWithAttributes(
+			semconv.ServiceNameKey.String(serviceName),
+			attribute.String(tagKey, tagVal),
+		)),
+	)
+
 	tracer := tp.Tracer("test-tracer")
+
 	ctx := context.Background()
 	for i := 0; i < 3; i++ {
 		_, span := tracer.Start(ctx, fmt.Sprintf("test-span-%d", i))
@@ -221,6 +233,7 @@ func TestExporterExportSpan(t *testing.T) {
 	}
 
 	require.NoError(t, tp.Shutdown(ctx))
+
 	batchesUploaded := testCollector.batchesUploaded
 	require.Len(t, batchesUploaded, 1)
 	uploadedBatch := batchesUploaded[0]
@@ -823,49 +836,3 @@ func TestProcess(t *testing.T) {
 		})
 	}
 }
-
-func TestNewExporterPipelineWithOptions(t *testing.T) {
-	const (
-		serviceName     = "test-service"
-		eventCountLimit = 10
-		tagKey          = "key"
-		tagVal          = "val"
-	)
-
-	testCollector := &testCollectorEndpoint{}
-	tp, err := NewExportPipeline(
-		withTestCollectorEndpointInjected(testCollector),
-		WithSDKOptions(
-			sdktrace.WithResource(resource.NewWithAttributes(
-				semconv.ServiceNameKey.String(serviceName),
-				attribute.String(tagKey, tagVal),
-			)),
-			sdktrace.WithSpanLimits(sdktrace.SpanLimits{
-				EventCountLimit: eventCountLimit,
-			}),
-		),
-	)
-	require.NoError(t, err)
-
-	ctx := context.Background()
-	_, span := tp.Tracer("test-tracer").Start(ctx, "test-span")
-	for i := 0; i < eventCountLimit*2; i++ {
-		span.AddEvent(fmt.Sprintf("event-%d", i))
-	}
-	span.End()
-	require.NoError(t, tp.Shutdown(ctx))
-
-	assert.True(t, span.SpanContext().IsValid())
-
-	batchesUploaded := testCollector.batchesUploaded
-	require.Len(t, batchesUploaded, 1)
-	uploadedBatch := batchesUploaded[0]
-	assert.Equal(t, serviceName, uploadedBatch.GetProcess().GetServiceName())
-	require.Len(t, uploadedBatch.GetSpans(), 1)
-	uploadedSpan := uploadedBatch.GetSpans()[0]
-	assert.Len(t, uploadedSpan.GetLogs(), eventCountLimit)
-
-	require.Len(t, uploadedBatch.GetProcess().GetTags(), 1)
-	assert.Equal(t, tagKey, uploadedBatch.GetProcess().GetTags()[0].GetKey())
-	assert.Equal(t, tagVal, uploadedBatch.GetProcess().GetTags()[0].GetVStr())
-}