From b0f70ff453cffef410c94aa954fb731061f1f4ef Mon Sep 17 00:00:00 2001
From: Shawn Poulson <92753637+Baliedge@users.noreply.github.com>
Date: Wed, 2 Mar 2022 09:47:27 -0500
Subject: [PATCH] Fix race condition in tracing package.  Update documentation.
 (#98)

---
 tracing/README.md  | 18 ++++++++++++++++++
 tracing/scope.go   | 11 +++++------
 tracing/tracing.go | 32 ++++++++++++++++++++++++++------
 3 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/tracing/README.md b/tracing/README.md
index fd17e659..8b599e18 100644
--- a/tracing/README.md
+++ b/tracing/README.md
@@ -87,6 +87,24 @@ tracing.SetDefaultTracer(tracer)
 tracing.CloseTracing(context.Background())
 ```
 
+### Tracer Lifecycle
+The common use case is to call `InitTracing()` to build a single default tracer
+that the application uses througout its lifetime, then call `CloseTracing()` on
+shutdown.
+
+The default tracer is stored globally in the tracer package for use by tracing
+functions.
+
+The tracer object identifies itself by a library name, which can be seen in Jaeger
+traces as attribute `otel.library.name`.  This value is typically the module
+name of the application.
+
+If it's necessary to create traces with a different library name, additional
+tracer objects may be created by `NewTracer()` which returns a context with the
+tracer object embedded in it.  This context object must be passed to tracing
+functions use that tracer in particular, otherwise the default tracer will be
+selected.
+
 ### Setting Resources
 OpenTelemetry is configured by environment variables and supplemental resource
 settings.  Some of these resources also map to environment variables.
diff --git a/tracing/scope.go b/tracing/scope.go
index eb76e0ce..e4a9759d 100644
--- a/tracing/scope.go
+++ b/tracing/scope.go
@@ -77,12 +77,11 @@ func startSpan(ctx context.Context, spanName, fileTag string, opts ...trace.Span
 	tracer, ok := ctx.Value(tracerKey{}).(trace.Tracer)
 	if !ok {
 		// No tracer embedded.  Fall back to default tracer.
-		tracer = defaultTracer
-
-		// Else, omit tracing.
-		if tracer == nil {
-			return ctx
-		}
+		tracer = GetDefaultTracer()
+	}
+	// Else, omit tracing.
+	if tracer == nil {
+		return ctx
 	}
 
 	opts = append(opts, trace.WithAttributes(
diff --git a/tracing/tracing.go b/tracing/tracing.go
index 4805f310..16784ba8 100644
--- a/tracing/tracing.go
+++ b/tracing/tracing.go
@@ -2,6 +2,7 @@ package tracing
 
 import (
 	"context"
+	"sync"
 	"time"
 
 	"github.com/mailgun/holster/v4/errors"
@@ -23,6 +24,7 @@ var logLevels = []logrus.Level{
 	logrus.TraceLevel,
 }
 
+var globalMutex sync.RWMutex
 var defaultTracer trace.Tracer
 
 // InitTracing initializes a global OpenTelemetry tracer provider singleton.
@@ -66,7 +68,9 @@ func InitTracing(ctx context.Context, libraryName string, opts ...sdktrace.Trace
 		return ctx, nil, errors.Wrap(err, "error in NewTracer")
 	}
 
-	SetDefaultTracer(tracer)
+	if GetDefaultTracer() == nil {
+		SetDefaultTracer(tracer)
+	}
 
 	// Required for trace propagation between services.
 	otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{}))
@@ -81,16 +85,25 @@ func InitTracing(ctx context.Context, libraryName string, opts ...sdktrace.Trace
 func NewTracer(ctx context.Context, libraryName string) (context.Context, trace.Tracer, error) {
 	tp, ok := otel.GetTracerProvider().(*sdktrace.TracerProvider)
 	if !ok {
-		return nil, nil, errors.New("OpenTelemetry global tracer provider has not be initialized")
+		return nil, nil, errors.New("OpenTelemetry global tracer provider has not been initialized")
 	}
 
 	tracer := tp.Tracer(libraryName)
-	ctx = context.WithValue(ctx, tracerKey{}, tracer)
+	ctx = ContextWithTracer(ctx, tracer)
 	return ctx, tracer, nil
 }
 
+// GetDefaultTracer gets the global tracer used as a default by this package.
+func GetDefaultTracer() trace.Tracer {
+	globalMutex.RLock()
+	defer globalMutex.RUnlock()
+	return defaultTracer
+}
+
 // SetDefaultTracer sets the global tracer used as a default by this package.
 func SetDefaultTracer(tracer trace.Tracer) {
+	globalMutex.Lock()
+	defer globalMutex.Unlock()
 	defaultTracer = tracer
 }
 
@@ -99,13 +112,13 @@ func SetDefaultTracer(tracer trace.Tracer) {
 func CloseTracing(ctx context.Context) error {
 	tp, ok := otel.GetTracerProvider().(*sdktrace.TracerProvider)
 	if !ok {
-		return errors.New("OpenTelemetry global tracer provider has not be initialized")
+		return errors.New("OpenTelemetry global tracer provider has not been initialized")
 	}
 
-	defaultTracer = nil
-
+	SetDefaultTracer(nil)
 	ctx, cancel := context.WithTimeout(ctx, 5 * time.Second)
 	defer cancel()
+
 	err := tp.Shutdown(ctx)
 	if err != nil {
 		return errors.Wrap(err, "error in tp.Shutdown")
@@ -114,6 +127,13 @@ func CloseTracing(ctx context.Context) error {
 	return nil
 }
 
+// ContextWithTracer creates a context with a tracer object embedded.
+// This value is used by scope functions or use TracerFromContext() to retrieve
+// it.
+func ContextWithTracer(ctx context.Context, tracer trace.Tracer) context.Context {
+	return context.WithValue(ctx, tracerKey{}, tracer)
+}
+
 // TracerFromContext gets embedded `Tracer` from context.
 // Returns nil if not found.
 func TracerFromContext(ctx context.Context) trace.Tracer {