Skip to content

trace: use weak in TracerProvider for storing tracers #6655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

trace: use weak in TracerProvider for storing tracers #6655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3370d53
use weak in TracerProvider for storing tracers
yumosx Apr 13, 2025
69676fd
refacotr it
yumosx Apr 16, 2025
c3ba006
refactor test: use rutime.GC
yumosx Apr 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions sdk/trace/benchmark_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,14 @@ package trace_test
import (
"context"
"fmt"
"runtime"
"sync"
"testing"
"time"

"go.opentelemetry.io/otel/sdk/instrumentation"
"go.opentelemetry.io/otel/trace/embedded"

"github.com/go-logr/logr"
"github.com/go-logr/logr/funcr"

Expand Down Expand Up @@ -389,3 +394,48 @@ func BenchmarkSpanProcessorVerboseLogging(b *testing.B) {
}
}
}

func BenchmarkTracerCleanup(b *testing.B) {
b.Run("WithWeakAndCleanup", func(b *testing.B) {
p := sdktrace.NewTracerProvider()

b.ReportAllocs()
b.ResetTimer()

for i := 0; i < b.N; i++ {
func() {
tr := p.Tracer(fmt.Sprintf("tracer-%d", i))
_ = tr
}()

if i%100 == 0 {
runtime.GC()
}
}
})

b.Run("NoWeak, NoCleanup", func(b *testing.B) {
Copy link
Contributor Author

@yumosx yumosx Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this test method is correct or reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this tests are reasonable, weak+cleanup works better based on the results.

BenchmarkTracerCleanup/WithWeakAndCleanup-8         	  691965	      1630 ns/op	     592 B/op	      13 allocs/op
BenchmarkTracerCleanup/NoWeak,_NoCleanup
BenchmarkTracerCleanup/NoWeak,_NoCleanup-8          	  304090	     91877 ns/op	     253 B/op	       5 allocs/op

type tracer struct {
embedded.Tracer
provider *sdktrace.TracerProvider
instrumentationScope instrumentation.Scope
}

p := sdktrace.NewTracerProvider()
var namedTracer sync.Map

b.ReportAllocs()
b.ResetTimer()

for i := 0; i < b.N; i++ {
func() {
name := fmt.Sprintf("tracer-%d", i)
scope := instrumentation.Scope{Name: name}
_, _ = namedTracer.LoadOrStore(name, &tracer{provider: p, instrumentationScope: scope})
}()
if i%100 == 0 {
runtime.GC()
}
}
})
}
32 changes: 20 additions & 12 deletions sdk/trace/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ package trace // import "go.opentelemetry.io/otel/sdk/trace"
import (
"context"
"fmt"
"runtime"
"sync"
"sync/atomic"
"weak"

"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/internal/global"
Expand Down Expand Up @@ -67,7 +69,7 @@ type TracerProvider struct {
embedded.TracerProvider

mu sync.Mutex
namedTracer map[instrumentation.Scope]*tracer
namedTracer sync.Map // map[instrumentation.Scope]weak.Pointer[tracer]
spanProcessors atomic.Pointer[spanProcessorStates]

isShutdown atomic.Bool
Expand Down Expand Up @@ -105,7 +107,6 @@ func NewTracerProvider(opts ...TracerProviderOption) *TracerProvider {
o = ensureValidTracerProviderConfig(o)

tp := &TracerProvider{
namedTracer: make(map[instrumentation.Scope]*tracer),
sampler: o.sampler,
idGenerator: o.idGenerator,
spanLimits: o.spanLimits,
Expand Down Expand Up @@ -146,22 +147,29 @@ func (p *TracerProvider) Tracer(name string, opts ...trace.TracerOption) trace.T
}

t, ok := func() (trace.Tracer, bool) {
p.mu.Lock()
defer p.mu.Unlock()
// Must check the flag after acquiring the mutex to avoid returning a valid tracer if Shutdown() ran
// after the first check above but before we acquired the mutex.
if p.isShutdown.Load() {
return noop.NewTracerProvider().Tracer(name, opts...), true
}
t, ok := p.namedTracer[is]
if !ok {
t = &tracer{
provider: p,
instrumentationScope: is,
}
p.namedTracer[is] = t

tracerPtr := &tracer{provider: p, instrumentationScope: is}
wp := weak.Make(tracerPtr) //nolint

value, loaded := p.namedTracer.LoadOrStore(is, wp)
if !loaded {
runtime.AddCleanup(tracerPtr, func(is instrumentation.Scope) { //nolint
p.namedTracer.CompareAndDelete(is, wp)
}, is)
return tracerPtr, loaded
}
return t, ok

if mf := value.(weak.Pointer[tracer]).Value(); mf != nil { //nolint
return mf, loaded
}

p.namedTracer.CompareAndDelete(is, value)
return noop.NewTracerProvider().Tracer(name, opts...), true
}()
if !ok {
// This code is outside the mutex to not hold the lock while calling third party logging code:
Expand Down
Loading