From 5325224d46979b173e478b6941436f20ec7a1a5a Mon Sep 17 00:00:00 2001 From: Sri Harsha CH Date: Thu, 8 Feb 2024 14:39:34 +0000 Subject: [PATCH 1/3] fix: add mutex to otel test variable --- internal/trace/trace.go | 5 +++++ spanner/test/opentelemetry/test/ot_traces_test.go | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/internal/trace/trace.go b/internal/trace/trace.go index eabed000f309..da60241f17f2 100644 --- a/internal/trace/trace.go +++ b/internal/trace/trace.go @@ -20,6 +20,7 @@ import ( "fmt" "os" "strings" + "sync" "go.opencensus.io/trace" "go.opentelemetry.io/otel" @@ -59,6 +60,8 @@ var ( // original value after each test. OpenTelemetryTracingEnabled bool = strings.EqualFold(strings.TrimSpace( os.Getenv(TelemetryPlatformTracingVar)), TelemetryPlatformTracingOpenTelemetry) + + OpenTelemetryTracingEnabledMu = sync.RWMutex{} ) // IsOpenCensusTracingEnabled returns true if the environment variable @@ -72,6 +75,8 @@ func IsOpenCensusTracingEnabled() bool { // GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING is set to the // case-insensitive value "opentelemetry". func IsOpenTelemetryTracingEnabled() bool { + OpenTelemetryTracingEnabledMu.RLock() + defer OpenTelemetryTracingEnabledMu.RUnlock() return OpenTelemetryTracingEnabled } diff --git a/spanner/test/opentelemetry/test/ot_traces_test.go b/spanner/test/opentelemetry/test/ot_traces_test.go index 23d267a0bca4..448a4f4fb4b4 100644 --- a/spanner/test/opentelemetry/test/ot_traces_test.go +++ b/spanner/test/opentelemetry/test/ot_traces_test.go @@ -33,10 +33,16 @@ import ( func TestSpannerTracesWithOpenTelemetry(t *testing.T) { ctx := context.Background() te := newOpenTelemetryTestExporter(false, true) + trace.OpenTelemetryTracingEnabledMu.RLock() old := trace.OpenTelemetryTracingEnabled + trace.OpenTelemetryTracingEnabledMu.RUnlock() + trace.OpenTelemetryTracingEnabledMu.Lock() trace.OpenTelemetryTracingEnabled = true + trace.OpenTelemetryTracingEnabledMu.Unlock() t.Cleanup(func() { + trace.OpenTelemetryTracingEnabledMu.Lock() + defer trace.OpenTelemetryTracingEnabledMu.Unlock() trace.OpenTelemetryTracingEnabled = old te.Unregister(ctx) }) From 84afed3c444b5c1761c5de3e9df3349b5255886c Mon Sep 17 00:00:00 2001 From: Sri Harsha CH Date: Thu, 8 Feb 2024 16:39:08 +0000 Subject: [PATCH 2/3] feat: code refactoring --- internal/trace/trace.go | 24 +++++++++++-------- internal/trace/trace_test.go | 12 +++++----- .../test/opentelemetry/test/ot_traces_test.go | 12 +++------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/internal/trace/trace.go b/internal/trace/trace.go index da60241f17f2..63d8ef56febe 100644 --- a/internal/trace/trace.go +++ b/internal/trace/trace.go @@ -51,19 +51,23 @@ const ( ) var ( + // + openTelemetryTracingEnabledMu = sync.RWMutex{} // OpenTelemetryTracingEnabled is true if the environment variable // GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING is set to the // case-insensitive value "opentelemetry". - // - // Do not access directly. Use instead IsOpenTelemetryTracingEnabled or - // IsOpenCensusTracingEnabled. Intended for use only in unit tests. Restore - // original value after each test. - OpenTelemetryTracingEnabled bool = strings.EqualFold(strings.TrimSpace( + openTelemetryTracingEnabled bool = strings.EqualFold(strings.TrimSpace( os.Getenv(TelemetryPlatformTracingVar)), TelemetryPlatformTracingOpenTelemetry) - - OpenTelemetryTracingEnabledMu = sync.RWMutex{} ) +// Do not invoke SetOpenTelemetryTracingEnabledField directly. Set GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING +// environment variable instead. Intended for use only in unit tests. Restore original value after each test. +func SetOpenTelemetryTracingEnabledField(enabled bool) { + openTelemetryTracingEnabledMu.Lock() + defer openTelemetryTracingEnabledMu.Unlock() + openTelemetryTracingEnabled = enabled +} + // IsOpenCensusTracingEnabled returns true if the environment variable // GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING is NOT set to the // case-insensitive value "opentelemetry". @@ -75,9 +79,9 @@ func IsOpenCensusTracingEnabled() bool { // GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING is set to the // case-insensitive value "opentelemetry". func IsOpenTelemetryTracingEnabled() bool { - OpenTelemetryTracingEnabledMu.RLock() - defer OpenTelemetryTracingEnabledMu.RUnlock() - return OpenTelemetryTracingEnabled + openTelemetryTracingEnabledMu.RLock() + defer openTelemetryTracingEnabledMu.RUnlock() + return openTelemetryTracingEnabled } // StartSpan adds a span to the trace with the given name. If IsOpenCensusTracingEnabled diff --git a/internal/trace/trace_test.go b/internal/trace/trace_test.go index bfc02e284ee3..2e03d2a89df0 100644 --- a/internal/trace/trace_test.go +++ b/internal/trace/trace_test.go @@ -41,11 +41,11 @@ var ( ) func TestStartSpan_OpenCensus(t *testing.T) { - old := OpenTelemetryTracingEnabled - OpenTelemetryTracingEnabled = false + old := IsOpenTelemetryTracingEnabled() + SetOpenTelemetryTracingEnabledField(false) te := testutil.NewTestExporter() t.Cleanup(func() { - OpenTelemetryTracingEnabled = old + SetOpenTelemetryTracingEnabledField(old) te.Unregister() }) @@ -95,12 +95,12 @@ func TestStartSpan_OpenCensus(t *testing.T) { } func TestStartSpan_OpenTelemetry(t *testing.T) { - old := OpenTelemetryTracingEnabled - OpenTelemetryTracingEnabled = true + old := IsOpenTelemetryTracingEnabled() + SetOpenTelemetryTracingEnabledField(true) ctx := context.Background() te := testutil.NewOpenTelemetryTestExporter() t.Cleanup(func() { - OpenTelemetryTracingEnabled = old + SetOpenTelemetryTracingEnabledField(old) te.Unregister(ctx) }) diff --git a/spanner/test/opentelemetry/test/ot_traces_test.go b/spanner/test/opentelemetry/test/ot_traces_test.go index 448a4f4fb4b4..c2a4ed942f98 100644 --- a/spanner/test/opentelemetry/test/ot_traces_test.go +++ b/spanner/test/opentelemetry/test/ot_traces_test.go @@ -33,17 +33,11 @@ import ( func TestSpannerTracesWithOpenTelemetry(t *testing.T) { ctx := context.Background() te := newOpenTelemetryTestExporter(false, true) - trace.OpenTelemetryTracingEnabledMu.RLock() - old := trace.OpenTelemetryTracingEnabled - trace.OpenTelemetryTracingEnabledMu.RUnlock() - trace.OpenTelemetryTracingEnabledMu.Lock() - trace.OpenTelemetryTracingEnabled = true - trace.OpenTelemetryTracingEnabledMu.Unlock() + old := trace.IsOpenTelemetryTracingEnabled() + trace.SetOpenTelemetryTracingEnabledField(true) t.Cleanup(func() { - trace.OpenTelemetryTracingEnabledMu.Lock() - defer trace.OpenTelemetryTracingEnabledMu.Unlock() - trace.OpenTelemetryTracingEnabled = old + trace.SetOpenTelemetryTracingEnabledField(old) te.Unregister(ctx) }) From af768f3eddd6f81cd90a359e7d27323e60d2e303 Mon Sep 17 00:00:00 2001 From: Sri Harsha CH Date: Thu, 8 Feb 2024 17:55:04 +0000 Subject: [PATCH 3/3] feat: comments refactoring --- internal/trace/trace.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/trace/trace.go b/internal/trace/trace.go index 63d8ef56febe..97738b2cbee2 100644 --- a/internal/trace/trace.go +++ b/internal/trace/trace.go @@ -51,17 +51,17 @@ const ( ) var ( - // + // openTelemetryTracingEnabledMu guards access to openTelemetryTracingEnabled field openTelemetryTracingEnabledMu = sync.RWMutex{} - // OpenTelemetryTracingEnabled is true if the environment variable + // openTelemetryTracingEnabled is true if the environment variable // GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING is set to the // case-insensitive value "opentelemetry". openTelemetryTracingEnabled bool = strings.EqualFold(strings.TrimSpace( os.Getenv(TelemetryPlatformTracingVar)), TelemetryPlatformTracingOpenTelemetry) ) -// Do not invoke SetOpenTelemetryTracingEnabledField directly. Set GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING -// environment variable instead. Intended for use only in unit tests. Restore original value after each test. +// SetOpenTelemetryTracingEnabledField programmatically sets the value provided by GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING for the purpose of unit testing. +// Do not invoke it directly. Intended for use only in unit tests. Restore original value after each test. func SetOpenTelemetryTracingEnabledField(enabled bool) { openTelemetryTracingEnabledMu.Lock() defer openTelemetryTracingEnabledMu.Unlock()