Update Jaeger exporter convenience functions (#1822)

* Update Jaeger exporter convenience functions * Update changelog PR numbers * Update CHANGELOG.md Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> * Revert removed raw exporter test Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
open-telemetry · Apr 20, 2021 · 081cc61 · 081cc61
1 parent 1b9f16d
commit 081cc61
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 106 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,7 +46,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   Additionally, this tag is overridden, as specified in the OTel specification, if the event contains an attribute with that key. (#1768)
 - Zipkin Exporter: Ensure mapping between OTel and Zipkin span data complies with the specification. (#1688)
 - Fixed typo for default service name in Jaeger Exporter. (#1797)
-- Fix flaky OTLP for the reconnnection of the client connection. (#1527, TBD)
+- Fix flaky OTLP for the reconnnection of the client connection. (#1527, #1814)
 
 ### Changed
 
@@ -72,7 +72,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Make `ExportSpans` in Jaeger Exporter honor context deadline. (#1773)
 - The `go.opentelemetry.io/otel/sdk/export/trace` package is merged into the `go.opentelemetry.io/otel/sdk/trace` package. (#1778)
 - The prometheus.InstallNewPipeline example is moved from comment to example test (#1796)
-- Convenience functions for stdout exporter have been updated to return the `TracerProvider` implementation and enable the shutdown of the exporter. (#1800)
+- The convenience functions for the stdout exporter have been updated to return the `TracerProvider` implementation and enable the shutdown of the exporter. (#1800)
+- Replace the flush function returned from the Jaeger exporter's convenience creation functions (`InstallNewPipeline` and `NewExportPipeline`) with the `TracerProvider` implementation they create.
+  This enables the caller to shutdown and flush using the related `TracerProvider` methods. (#1822)
 
 ### Removed
 

diff --git a/example/jaeger/main.go b/example/jaeger/main.go
@@ -29,10 +29,9 @@ import (
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 )
 
-// initTracer creates a new trace provider instance and registers it as global trace provider.
-func initTracer() func() {
-	// Create and install Jaeger export pipeline.
-	flush, err := jaeger.InstallNewPipeline(
+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()),
@@ -46,23 +45,23 @@ func initTracer() func() {
 	if err != nil {
 		log.Fatal(err)
 	}
-	return flush
-}
-
-func main() {
-	ctx := context.Background()
+	defer func() {
+		if err := tp.Shutdown(context.Background()); err != nil {
+			log.Fatal(err)
+		}
+	}()
 
-	flush := initTracer()
-	defer flush()
+	tr := tp.Tracer("component-main")
 
-	tr := otel.Tracer("component-main")
+	ctx := context.Background()
 	ctx, span := tr.Start(ctx, "foo")
 	defer span.End()
 
 	bar(ctx)
 }
 
 func bar(ctx context.Context) {
+	// Use the global TracerProvider.
 	tr := otel.Tracer("component-bar")
 	_, span := tr.Start(ctx, "bar")
 	span.SetAttributes(attribute.Key("testset").String("value"))

diff --git a/exporters/trace/jaeger/jaeger.go b/exporters/trace/jaeger/jaeger.go
@@ -132,32 +132,32 @@ 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) (trace.TracerProvider, func(), error) {
+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, nil, err
+		return nil, err
 	}
 
 	pOpts := append(o.TracerProviderOptions, sdktrace.WithSyncer(exporter))
 	tp := sdktrace.NewTracerProvider(pOpts...)
-	return tp, exporter.Flush, nil
+	return tp, nil
 }
 
 // InstallNewPipeline instantiates a NewExportPipeline with the
 // recommended configuration and registers it globally.
-func InstallNewPipeline(endpointOption EndpointOption, opts ...Option) (func(), error) {
-	tp, flushFn, err := NewExportPipeline(endpointOption, opts...)
+func InstallNewPipeline(endpointOption EndpointOption, opts ...Option) (*sdktrace.TracerProvider, error) {
+	tp, err := NewExportPipeline(endpointOption, opts...)
 	if err != nil {
-		return nil, err
+		return tp, err
 	}
 
 	otel.SetTracerProvider(tp)
-	return flushFn, nil
+	return tp, nil
 }
 
 // Exporter is an implementation of an OTel SpanSyncer that uploads spans to

diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go
@@ -46,93 +46,33 @@ const (
 )
 
 func TestInstallNewPipeline(t *testing.T) {
-	testCases := []struct {
-		name             string
-		endpoint         EndpointOption
-		options          []Option
-		expectedProvider trace.TracerProvider
-	}{
-		{
-			name:             "simple pipeline",
-			endpoint:         WithCollectorEndpoint(collectorEndpoint),
-			expectedProvider: &sdktrace.TracerProvider{},
-		},
-		{
-			name:             "with agent endpoint",
-			endpoint:         WithAgentEndpoint(),
-			expectedProvider: &sdktrace.TracerProvider{},
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			fn, err := InstallNewPipeline(
-				tc.endpoint,
-				tc.options...,
-			)
-			defer fn()
-
-			assert.NoError(t, err)
-			assert.IsType(t, tc.expectedProvider, otel.GetTracerProvider())
-
-			otel.SetTracerProvider(nil)
-		})
-	}
+	tp, err := InstallNewPipeline(WithCollectorEndpoint(collectorEndpoint))
+	require.NoError(t, err)
+	// Ensure InstallNewPipeline sets the global TracerProvider. By default
+	// the global tracer provider will be a NoOp implementation, this checks
+	// if that has been overwritten.
+	assert.IsType(t, tp, otel.GetTracerProvider())
 }
 
-func TestNewExportPipeline(t *testing.T) {
+func TestNewRawExporterOptions(t *testing.T) {
 	testCases := []struct {
-		name                                  string
-		endpoint                              EndpointOption
-		options                               []Option
-		expectedProviderType                  trace.TracerProvider
-		testSpanSampling, spanShouldBeSampled bool
+		name     string
+		endpoint EndpointOption
 	}{
 		{
-			name:                 "simple pipeline",
-			endpoint:             WithCollectorEndpoint(collectorEndpoint),
-			expectedProviderType: &sdktrace.TracerProvider{},
-		},
-		{
-			name:     "always on",
+			name:     "default exporter with collector endpoint",
 			endpoint: WithCollectorEndpoint(collectorEndpoint),
-			options: []Option{
-				WithSDKOptions(sdktrace.WithSampler(sdktrace.AlwaysSample())),
-			},
-			expectedProviderType: &sdktrace.TracerProvider{},
-			testSpanSampling:     true,
-			spanShouldBeSampled:  true,
 		},
 		{
-			name:     "never",
-			endpoint: WithCollectorEndpoint(collectorEndpoint),
-			options: []Option{
-				WithSDKOptions(sdktrace.WithSampler(sdktrace.NeverSample())),
-			},
-			expectedProviderType: &sdktrace.TracerProvider{},
-			testSpanSampling:     true,
-			spanShouldBeSampled:  false,
+			name:     "default exporter with agent endpoint",
+			endpoint: WithAgentEndpoint(),
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			tp, fn, err := NewExportPipeline(
-				tc.endpoint,
-				tc.options...,
-			)
-			defer fn()
-
+			_, err := NewRawExporter(tc.endpoint)
 			assert.NoError(t, err)
-			assert.NotEqual(t, tp, otel.GetTracerProvider())
-			assert.IsType(t, tc.expectedProviderType, tp)
-
-			if tc.testSpanSampling {
-				_, span := tp.Tracer("jaeger test").Start(context.Background(), tc.name)
-				spanCtx := span.SpanContext()
-				assert.Equal(t, tc.spanShouldBeSampled, spanCtx.IsSampled())
-				span.End()
-			}
 		})
 	}
 }
@@ -254,15 +194,15 @@ func withTestCollectorEndpointInjected(ce *testCollectorEndpoint) func() (batchU
 	}
 }
 
-func TestExporter_ExportSpan(t *testing.T) {
+func TestExporterExportSpan(t *testing.T) {
 	const (
 		serviceName = "test-service"
 		tagKey      = "key"
 		tagVal      = "val"
 	)
 
 	testCollector := &testCollectorEndpoint{}
-	tp, spanFlush, err := NewExportPipeline(
+	tp, err := NewExportPipeline(
 		withTestCollectorEndpointInjected(testCollector),
 		WithSDKOptions(
 			sdktrace.WithResource(resource.NewWithAttributes(
@@ -271,16 +211,16 @@ func TestExporter_ExportSpan(t *testing.T) {
 			)),
 		),
 	)
-	assert.NoError(t, err)
-	otel.SetTracerProvider(tp)
-	tracer := otel.Tracer("test-tracer")
+	require.NoError(t, err)
+	tracer := tp.Tracer("test-tracer")
+	ctx := context.Background()
 	for i := 0; i < 3; i++ {
-		_, span := tracer.Start(context.Background(), fmt.Sprintf("test-span-%d", i))
+		_, span := tracer.Start(ctx, fmt.Sprintf("test-span-%d", i))
 		span.End()
 		assert.True(t, span.SpanContext().IsValid())
 	}
 
-	spanFlush()
+	require.NoError(t, tp.Shutdown(ctx))
 	batchesUploaded := testCollector.batchesUploaded
 	require.Len(t, batchesUploaded, 1)
 	uploadedBatch := batchesUploaded[0]
@@ -893,7 +833,7 @@ func TestNewExporterPipelineWithOptions(t *testing.T) {
 	)
 
 	testCollector := &testCollectorEndpoint{}
-	tp, spanFlush, err := NewExportPipeline(
+	tp, err := NewExportPipeline(
 		withTestCollectorEndpointInjected(testCollector),
 		WithSDKOptions(
 			sdktrace.WithResource(resource.NewWithAttributes(
@@ -905,15 +845,15 @@ func TestNewExporterPipelineWithOptions(t *testing.T) {
 			}),
 		),
 	)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
-	otel.SetTracerProvider(tp)
-	_, span := otel.Tracer("test-tracer").Start(context.Background(), "test-span")
+	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()
-	spanFlush()
+	require.NoError(t, tp.Shutdown(ctx))
 
 	assert.True(t, span.SpanContext().IsValid())