Unify instrumentation *Provider options (#303)

* Unify instrumentation options for mongo-driver * Unify instrumentation options for gin * Unify instrumentation options for gorilla * Unify instrumentation options for labstack echo * Unify instrumentation options for go-restful * Unify instrumentation options for gomemcache * Unify instrumentation options for sarama * Unify instrumentation options for net/http * Unify instrumentation options for beego * Update instrumentation guidelines about uniform *Provider options * update CHANGELOG * Update guidelines * Fix naming and remove unnecessary fields * Avoid the escalation of a test failure to panic * Make config struct of instrumentation unexported * Update style guide * Update CHANGELOG Co-authored-by: Tyler Yahn <[email protected]>
open-telemetry · Sep 1, 2020 · faaf7c0 · faaf7c0
1 parent dc7145c
commit faaf7c0
Show file tree

Hide file tree

Showing 40 changed files with 353 additions and 329 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,13 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Benchmark tests for the gRPC instrumentation. (#296)
 - Integration testing for the gRPC instrumentation. (#297)
 
+### Changed
+
+- Unify instrumentation about provider options for `go.mongodb.org/mongo-driver`, `gin-gonic/gin`, `gorilla/mux`,
+  `labstack/echo`, `emicklei/go-restful`, `bradfitz/gomemcache`, `Shopify/sarama`, `net/http` and `beego`. (#303)
+- Update instrumentation guidelines about uniform provider options. Also, update style guide. (#303)
+- Make config struct of instrumentation unexported. (#303)
+
 ## [0.11.0] - 2020-08-25
 
 ### Added

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -91,6 +91,7 @@ Any Maintainer can merge the PR once it is **ready to merge**.
 
 * Make sure to run `make precommit` - this will find and fix the code
   formatting.
+* Check [opentelemetry-go Style Guide](https://github.com/open-telemetry/opentelemetry-go/blob/master/CONTRIBUTING.md#style-guide)
 
 ## Adding a new Contrib package
 

diff --git a/instrumentation/README.md b/instrumentation/README.md
@@ -35,6 +35,10 @@ Additionally the following guidelines for package composition need to be followe
    This documentation SHOULD be in a dedicated `doc.go` file if the package is more than one file.
    It SHOULD contain useful information like what the purpose of the instrumentation is, how to use it, and any compatibility restrictions that might exist. 
 - Examples of how to actually use the instrumentation SHOULD be included.
+- All instrumentation packages MUST provide an option to accept a `TracerProvider` if it uses a Tracer, a `MeterProvider` if it uses a Meter, and `Propagators` if it handles any context propagation.
+  Also, packages MUST use the default `TracerProvider`, `MeterProvider`, and `Propagators` supplied by the `global` package if no optional one is provided.
+- All instrumentation packages MUST NOT provide an option to accept a `Tracer` or `Meter`.
+- All instrumentation packages MUST create any used `Tracer` or `Meter` with a name matching the instrumentation package name.
 
 ## Additional Instrumentation Packages
 

diff --git a/instrumentation/github.com/Shopify/sarama/consumer_test.go b/instrumentation/github.com/Shopify/sarama/consumer_test.go
@@ -53,7 +53,7 @@ func TestWrapPartitionConsumer(t *testing.T) {
 	partitionConsumer, err := consumer.ConsumePartition(topic, 0, 0)
 	require.NoError(t, err)
 
-	partitionConsumer = WrapPartitionConsumer(partitionConsumer, WithTraceProvider(provider))
+	partitionConsumer = WrapPartitionConsumer(partitionConsumer, WithTracerProvider(provider))
 
 	consumeAndCheck(t, mt, mockPartitionConsumer, partitionConsumer)
 }
@@ -67,7 +67,7 @@ func TestWrapConsumer(t *testing.T) {
 	mockPartitionConsumer := mockConsumer.ExpectConsumePartition(topic, 0, 0)
 
 	// Wrap consumer
-	consumer := WrapConsumer(mockConsumer, WithTraceProvider(provider))
+	consumer := WrapConsumer(mockConsumer, WithTracerProvider(provider))
 
 	// Create partition consumer
 	partitionConsumer, err := consumer.ConsumePartition(topic, 0, 0)
@@ -170,7 +170,7 @@ func BenchmarkWrapPartitionConsumer(b *testing.B) {
 
 	mockPartitionConsumer, partitionConsumer := createMockPartitionConsumer(b)
 
-	partitionConsumer = WrapPartitionConsumer(partitionConsumer, WithTraceProvider(provider))
+	partitionConsumer = WrapPartitionConsumer(partitionConsumer, WithTracerProvider(provider))
 	message := sarama.ConsumerMessage{Key: []byte("foo")}
 
 	b.ReportAllocs()

diff --git a/instrumentation/github.com/Shopify/sarama/option.go b/instrumentation/github.com/Shopify/sarama/option.go
@@ -28,35 +28,35 @@ const (
 )
 
 type config struct {
-	TraceProvider trace.Provider
-	Propagators   otelpropagation.Propagators
+	TracerProvider trace.Provider
+	Propagators    otelpropagation.Propagators
 
 	Tracer trace.Tracer
 }
 
 // newConfig returns a config with all Options set.
 func newConfig(opts ...Option) config {
 	cfg := config{
-		Propagators:   global.Propagators(),
-		TraceProvider: global.TraceProvider(),
+		Propagators:    global.Propagators(),
+		TracerProvider: global.TraceProvider(),
 	}
 	for _, opt := range opts {
 		opt(&cfg)
 	}
 
-	cfg.Tracer = cfg.TraceProvider.Tracer(defaultTracerName)
+	cfg.Tracer = cfg.TracerProvider.Tracer(defaultTracerName)
 
 	return cfg
 }
 
 // Option specifies instrumentation configuration options.
 type Option func(*config)
 
-// WithTraceProvider specifies a trace provider to use for creating a tracer for spans.
+// WithTracerProvider specifies a tracer provider to use for creating a tracer.
 // If none is specified, the global provider is used.
-func WithTraceProvider(provider trace.Provider) Option {
+func WithTracerProvider(provider trace.Provider) Option {
 	return func(cfg *config) {
-		cfg.TraceProvider = provider
+		cfg.TracerProvider = provider
 	}
 }
 

diff --git a/instrumentation/github.com/Shopify/sarama/option_test.go b/instrumentation/github.com/Shopify/sarama/option_test.go
@@ -31,12 +31,12 @@ func TestNewConfig(t *testing.T) {
 		{
 			name: "with provider",
 			opts: []Option{
-				WithTraceProvider(global.TraceProvider()),
+				WithTracerProvider(global.TraceProvider()),
 			},
 			expected: config{
-				TraceProvider: global.TraceProvider(),
-				Tracer:        global.TraceProvider().Tracer(defaultTracerName),
-				Propagators:   global.Propagators(),
+				TracerProvider: global.TraceProvider(),
+				Tracer:         global.TraceProvider().Tracer(defaultTracerName),
+				Propagators:    global.Propagators(),
 			},
 		},
 		{
@@ -45,9 +45,9 @@ func TestNewConfig(t *testing.T) {
 				WithPropagators(nil),
 			},
 			expected: config{
-				TraceProvider: global.TraceProvider(),
-				Tracer:        global.TraceProvider().Tracer(defaultTracerName),
-				Propagators:   nil,
+				TracerProvider: global.TraceProvider(),
+				Tracer:         global.TraceProvider().Tracer(defaultTracerName),
+				Propagators:    nil,
 			},
 		},
 	}

diff --git a/instrumentation/github.com/Shopify/sarama/producer_test.go b/instrumentation/github.com/Shopify/sarama/producer_test.go
@@ -52,7 +52,7 @@ func TestWrapSyncProducer(t *testing.T) {
 	mockSyncProducer := mocks.NewSyncProducer(t, cfg)
 
 	// Wrap sync producer
-	syncProducer := WrapSyncProducer(cfg, mockSyncProducer, WithTraceProvider(provider))
+	syncProducer := WrapSyncProducer(cfg, mockSyncProducer, WithTracerProvider(provider))
 
 	// Create message with span context
 	ctx, _ := mt.Start(context.Background(), "")
@@ -167,7 +167,7 @@ func TestWrapAsyncProducer(t *testing.T) {
 
 		cfg := newSaramaConfig()
 		mockAsyncProducer := mocks.NewAsyncProducer(t, cfg)
-		ap := WrapAsyncProducer(cfg, mockAsyncProducer, WithTraceProvider(provider))
+		ap := WrapAsyncProducer(cfg, mockAsyncProducer, WithTracerProvider(provider))
 
 		msgList := createMessages(mt)
 		// Send message
@@ -237,7 +237,7 @@ func TestWrapAsyncProducer(t *testing.T) {
 		cfg.Producer.Return.Successes = true
 
 		mockAsyncProducer := mocks.NewAsyncProducer(t, cfg)
-		ap := WrapAsyncProducer(cfg, mockAsyncProducer, WithTraceProvider(provider))
+		ap := WrapAsyncProducer(cfg, mockAsyncProducer, WithTracerProvider(provider))
 
 		msgList := createMessages(mt)
 		// Send message
@@ -315,7 +315,7 @@ func TestWrapAsyncProducerError(t *testing.T) {
 	cfg.Producer.Return.Successes = true
 
 	mockAsyncProducer := mocks.NewAsyncProducer(t, cfg)
-	ap := WrapAsyncProducer(cfg, mockAsyncProducer, WithTraceProvider(provider))
+	ap := WrapAsyncProducer(cfg, mockAsyncProducer, WithTracerProvider(provider))
 
 	mockAsyncProducer.ExpectInputAndFail(errors.New("test"))
 	ap.Input() <- &sarama.ProducerMessage{Topic: topic, Key: sarama.StringEncoder("foo2")}
@@ -349,7 +349,7 @@ func BenchmarkWrapSyncProducer(b *testing.B) {
 	mockSyncProducer := mocks.NewSyncProducer(b, cfg)
 
 	// Wrap sync producer
-	syncProducer := WrapSyncProducer(cfg, mockSyncProducer, WithTraceProvider(provider))
+	syncProducer := WrapSyncProducer(cfg, mockSyncProducer, WithTracerProvider(provider))
 	message := sarama.ProducerMessage{Key: sarama.StringEncoder("foo")}
 
 	b.ReportAllocs()
@@ -390,7 +390,7 @@ func BenchmarkWrapAsyncProducer(b *testing.B) {
 	mockAsyncProducer := mocks.NewAsyncProducer(b, cfg)
 
 	// Wrap sync producer
-	asyncProducer := WrapAsyncProducer(cfg, mockAsyncProducer, WithTraceProvider(provider))
+	asyncProducer := WrapAsyncProducer(cfg, mockAsyncProducer, WithTracerProvider(provider))
 	message := sarama.ProducerMessage{Key: sarama.StringEncoder("foo")}
 
 	b.ReportAllocs()

diff --git a/instrumentation/github.com/astaxie/beego/beego.go b/instrumentation/github.com/astaxie/beego/beego.go
@@ -67,8 +67,8 @@ func NewOTelBeegoMiddleWare(service string, options ...Option) beego.MiddleWare
 	cfg := configure(options...)
 
 	httpOptions := []otelhttp.Option{
-		otelhttp.WithTracer(cfg.traceProvider.Tracer(packageName)),
-		otelhttp.WithMeter(cfg.meterProvider.Meter(packageName)),
+		otelhttp.WithTracerProvider(cfg.tracerProvider),
+		otelhttp.WithMeterProvider(cfg.meterProvider),
 		otelhttp.WithPropagators(cfg.propagators),
 	}
 

diff --git a/instrumentation/github.com/astaxie/beego/beego_test.go b/instrumentation/github.com/astaxie/beego/beego_test.go
@@ -43,16 +43,16 @@ import (
 
 // ------------------------------------------ Mock Trace Provider
 
-type MockTraceProvider struct {
+type MockTracerProvider struct {
 	tracer *mocktrace.Tracer
 }
 
-func (m *MockTraceProvider) Tracer(name string, options ...trace.TracerOption) trace.Tracer {
+func (m *MockTracerProvider) Tracer(name string, options ...trace.TracerOption) trace.Tracer {
 	return m.tracer
 }
 
-func NewTraceProvider() *MockTraceProvider {
-	return &MockTraceProvider{
+func NewTracerProvider() *MockTracerProvider {
+	return &MockTracerProvider{
 		tracer: mocktrace.NewTracer(packageName),
 	}
 }
@@ -205,7 +205,7 @@ func TestSpanFromContextDefaultProvider(t *testing.T) {
 	defer replaceBeego()
 	_, provider := mockmeter.NewProvider()
 	global.SetMeterProvider(provider)
-	global.SetTraceProvider(NewTraceProvider())
+	global.SetTraceProvider(NewTracerProvider())
 	router := beego.NewControllerRegister()
 	router.Get("/hello-with-span", func(ctx *beegoCtx.Context) {
 		assertSpanFromContext(ctx.Request.Context(), t)
@@ -238,7 +238,7 @@ func TestSpanFromContextCustomProvider(t *testing.T) {
 
 	mw := NewOTelBeegoMiddleWare(
 		middleWareName,
-		WithTraceProvider(NewTraceProvider()),
+		WithTracerProvider(NewTracerProvider()),
 		WithMeterProvider(provider),
 	)
 
@@ -249,7 +249,7 @@ func TestSpanFromContextCustomProvider(t *testing.T) {
 
 func TestStatic(t *testing.T) {
 	defer replaceBeego()
-	traceProvider := NewTraceProvider()
+	tracerProvider := NewTracerProvider()
 	meterimpl, meterProvider := mockmeter.NewProvider()
 	file, err := ioutil.TempFile("", "static-*.html")
 	require.NoError(t, err)
@@ -261,7 +261,7 @@ func TestStatic(t *testing.T) {
 	defer beego.SetStaticPath("/", "")
 
 	mw := NewOTelBeegoMiddleWare(middleWareName,
-		WithTraceProvider(traceProvider),
+		WithTracerProvider(tracerProvider),
 		WithMeterProvider(meterProvider),
 	)
 
@@ -278,7 +278,7 @@ func TestStatic(t *testing.T) {
 	body, err := ioutil.ReadAll(rr.Result().Body)
 	require.NoError(t, err)
 	require.Equal(t, "<h1>Hello, world!</h1>", string(body))
-	spans := traceProvider.tracer.EndedSpans()
+	spans := tracerProvider.tracer.EndedSpans()
 	require.Len(t, spans, 1)
 	assertSpan(t, spans[0], tc)
 	assertMetrics(t, meterimpl.MeasurementBatches, tc)
@@ -308,11 +308,11 @@ func TestRender(t *testing.T) {
 	beego.SetViewsPath(dir)
 	_, tplName = filepath.Split(file.Name())
 
-	traceProvider := NewTraceProvider()
+	tracerProvider := NewTracerProvider()
 
 	mw := NewOTelBeegoMiddleWare(
 		middleWareName,
-		WithTraceProvider(traceProvider),
+		WithTracerProvider(tracerProvider),
 	)
 	for _, str := range []string{"/render", "/renderstring", "/renderbytes"} {
 		rr := httptest.NewRecorder()
@@ -324,7 +324,7 @@ func TestRender(t *testing.T) {
 		require.NoError(t, err)
 	}
 
-	spans := traceProvider.tracer.EndedSpans()
+	spans := tracerProvider.tracer.EndedSpans()
 	require.Len(t, spans, 6) // 3 HTTP requests, each creating 2 spans
 	for _, span := range spans {
 		switch span.Name {
@@ -347,7 +347,7 @@ func TestRender(t *testing.T) {
 // ------------------------------------------ Utilities
 
 func runTest(t *testing.T, tc *testCase, url string) {
-	traceProvider := NewTraceProvider()
+	tracerProvider := NewTracerProvider()
 	meterimpl, meterProvider := mockmeter.NewProvider()
 	addTestRoutes(t)
 	defer replaceBeego()
@@ -366,7 +366,7 @@ func runTest(t *testing.T, tc *testCase, url string) {
 		middleWareName,
 		append(
 			tc.options,
-			WithTraceProvider(traceProvider),
+			WithTracerProvider(tracerProvider),
 			WithMeterProvider(meterProvider),
 		)...,
 	)
@@ -380,7 +380,7 @@ func runTest(t *testing.T, tc *testCase, url string) {
 	require.NoError(t, json.Unmarshal(body, &message))
 	require.Equal(t, tc.expectedResponse, message)
 
-	spans := traceProvider.tracer.EndedSpans()
+	spans := tracerProvider.tracer.EndedSpans()
 	if tc.hasSpan {
 		require.Len(t, spans, 1)
 		assertSpan(t, spans[0], tc)