From f41f985605c4b0547abccb87bafe9bdd114d643e Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 19 Apr 2021 13:31:11 -0700 Subject: [PATCH 1/3] Remove the WithSDKOptions from the Jaeger exporter --- CHANGELOG.md | 4 + example/jaeger/main.go | 11 -- exporters/trace/jaeger/jaeger.go | 23 ++--- exporters/trace/jaeger/jaeger_test.go | 143 +++++++------------------- 4 files changed, 48 insertions(+), 133 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e730195f060..556a66816a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,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. (TBD) ## [0.19.0] - 2021-03-18 diff --git a/example/jaeger/main.go b/example/jaeger/main.go index 832f7ac4f06..a18b348217c 100644 --- a/example/jaeger/main.go +++ b/example/jaeger/main.go @@ -21,12 +21,9 @@ import ( "log" "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" ) // initTracer creates a new trace provider instance and registers it as global trace provider. @@ -34,14 +31,6 @@ func initTracer() func() { // Create and install Jaeger export pipeline. flush, 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), - )), - ), ) if err != nil { log.Fatal(err) diff --git a/exporters/trace/jaeger/jaeger.go b/exporters/trace/jaeger/jaeger.go index a66511bae21..861b6569a4c 100644 --- a/exporters/trace/jaeger/jaeger.go +++ b/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) (trace.TracerProvider, func(), error) { - o := options{} - for _, opt := range opts { - opt(&o) - } - exporter, err := NewRawExporter(endpointOption, opts...) if err != nil { return nil, 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, exporter.Flush, nil } diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go index 8b1814baa14..e28c1905702 100644 --- a/exporters/trace/jaeger/jaeger_test.go +++ b/exporters/trace/jaeger/jaeger_test.go @@ -17,6 +17,7 @@ package jaeger import ( "context" "encoding/binary" + "errors" "fmt" "os" "sort" @@ -80,59 +81,35 @@ func TestInstallNewPipeline(t *testing.T) { } } -func TestNewExportPipeline(t *testing.T) { - testCases := []struct { - name string - endpoint EndpointOption - options []Option - expectedProviderType trace.TracerProvider - testSpanSampling, spanShouldBeSampled bool +func TestNewExportPipelinePassthroughError(t *testing.T) { + for _, testcase := range []struct { + name string + failing bool + epo EndpointOption }{ { - name: "simple pipeline", - endpoint: WithCollectorEndpoint(collectorEndpoint), - expectedProviderType: &sdktrace.TracerProvider{}, + name: "failing underlying NewRawExporter", + failing: true, + epo: func() (batchUploader, error) { + return nil, errors.New("error") + }, }, { - name: "always on", - endpoint: WithCollectorEndpoint(collectorEndpoint), - options: []Option{ - WithSDKOptions(sdktrace.WithSampler(sdktrace.AlwaysSample())), - }, - expectedProviderType: &sdktrace.TracerProvider{}, - testSpanSampling: true, - spanShouldBeSampled: true, + name: "with default agent endpoint", + epo: WithAgentEndpoint(), }, { - name: "never", - endpoint: WithCollectorEndpoint(collectorEndpoint), - options: []Option{ - WithSDKOptions(sdktrace.WithSampler(sdktrace.NeverSample())), - }, - expectedProviderType: &sdktrace.TracerProvider{}, - testSpanSampling: true, - spanShouldBeSampled: false, + name: "with collector endpoint", + epo: WithCollectorEndpoint(collectorEndpoint), }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - tp, fn, err := NewExportPipeline( - tc.endpoint, - tc.options..., - ) - defer fn() - - 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() + } { + t.Run(testcase.name, func(t *testing.T) { + _, _, err := NewExportPipeline(testcase.epo) + if testcase.failing { + require.Error(t, err) + return } + require.NoError(t, err) }) } } @@ -254,7 +231,7 @@ func withTestCollectorEndpointInjected(ce *testCollectorEndpoint) func() (batchU } } -func TestExporter_ExportSpan(t *testing.T) { +func TestExporterExportSpan(t *testing.T) { const ( serviceName = "test-service" tagKey = "key" @@ -262,25 +239,27 @@ func TestExporter_ExportSpan(t *testing.T) { ) testCollector := &testCollectorEndpoint{} - tp, spanFlush, 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), + )), ) - assert.NoError(t, err) - otel.SetTracerProvider(tp) - tracer := otel.Tracer("test-tracer") + + 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() + tp.Shutdown(ctx) + batchesUploaded := testCollector.batchesUploaded require.Len(t, batchesUploaded, 1) uploadedBatch := batchesUploaded[0] @@ -883,49 +862,3 @@ func TestProcess(t *testing.T) { }) } } - -func TestNewExporterPipelineWithOptions(t *testing.T) { - const ( - serviceName = "test-service" - eventCountLimit = 10 - tagKey = "key" - tagVal = "val" - ) - - testCollector := &testCollectorEndpoint{} - tp, spanFlush, err := NewExportPipeline( - withTestCollectorEndpointInjected(testCollector), - WithSDKOptions( - sdktrace.WithResource(resource.NewWithAttributes( - semconv.ServiceNameKey.String(serviceName), - attribute.String(tagKey, tagVal), - )), - sdktrace.WithSpanLimits(sdktrace.SpanLimits{ - EventCountLimit: eventCountLimit, - }), - ), - ) - assert.NoError(t, err) - - otel.SetTracerProvider(tp) - _, span := otel.Tracer("test-tracer").Start(context.Background(), "test-span") - for i := 0; i < eventCountLimit*2; i++ { - span.AddEvent(fmt.Sprintf("event-%d", i)) - } - span.End() - spanFlush() - - 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()) -} From 9d5aaeda83fd0c4d2ed65424730bef19e5f6ce58 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 19 Apr 2021 13:41:09 -0700 Subject: [PATCH 2/3] Lint --- CHANGELOG.md | 2 +- example/jaeger/go.mod | 1 - exporters/trace/jaeger/jaeger_test.go | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 556a66816a4..97eeda927ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,7 +96,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - 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. (TBD) + 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 diff --git a/example/jaeger/go.mod b/example/jaeger/go.mod index 087c386dad8..134a5e9e30b 100644 --- a/example/jaeger/go.mod +++ b/example/jaeger/go.mod @@ -11,7 +11,6 @@ replace ( require ( go.opentelemetry.io/otel v0.19.0 go.opentelemetry.io/otel/exporters/trace/jaeger v0.19.0 - go.opentelemetry.io/otel/sdk v0.19.0 ) replace go.opentelemetry.io/otel/bridge/opencensus => ../../bridge/opencensus diff --git a/exporters/trace/jaeger/jaeger_test.go b/exporters/trace/jaeger/jaeger_test.go index e28c1905702..a3f76af7fdf 100644 --- a/exporters/trace/jaeger/jaeger_test.go +++ b/exporters/trace/jaeger/jaeger_test.go @@ -258,7 +258,7 @@ func TestExporterExportSpan(t *testing.T) { assert.True(t, span.SpanContext().IsValid()) } - tp.Shutdown(ctx) + require.NoError(t, tp.Shutdown(ctx)) batchesUploaded := testCollector.batchesUploaded require.Len(t, batchesUploaded, 1) From 031bf2ca4c66f588ce9d92780f7b206fa4818fcf Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 20 Apr 2021 15:13:58 -0700 Subject: [PATCH 3/3] Setup SDK in Jaeger example --- example/jaeger/go.mod | 1 + example/jaeger/main.go | 57 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/example/jaeger/go.mod b/example/jaeger/go.mod index 134a5e9e30b..087c386dad8 100644 --- a/example/jaeger/go.mod +++ b/example/jaeger/go.mod @@ -11,6 +11,7 @@ replace ( require ( go.opentelemetry.io/otel v0.19.0 go.opentelemetry.io/otel/exporters/trace/jaeger v0.19.0 + go.opentelemetry.io/otel/sdk v0.19.0 ) replace go.opentelemetry.io/otel/bridge/opencensus => ../../bridge/opencensus diff --git a/example/jaeger/main.go b/example/jaeger/main.go index 8dc53de5385..f018852db11 100644 --- a/example/jaeger/main.go +++ b/example/jaeger/main.go @@ -19,30 +19,71 @@ package main import ( "context" "log" + "time" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/exporters/trace/jaeger" + "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"), +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()