Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
1 change: 0 additions & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ linters:
disabled-checks:
- appendAssign
- exitAfterDefer
- exposedSyncMutex
- hugeParam
- unnamedResult
enable-all: true
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Deprecated

- Deprecate direct exposure and usage of mutex objects via structs: introduce equivalent methods for locking/unlocking, and mark mutex fields as deprecated, specially for `go.opentelemetry.io/contrib/samplers/jaegerremote.Sampler` and `go.opentelemetry.io/contrib/propagators/aws/xray.IDGenerator`. (#7726)

### Removed

- Drop support for [Go 1.23]. (#7831)
Expand Down Expand Up @@ -57,6 +61,7 @@ The next release will require at least [Go 1.24].
- `Extract` and `Inject` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` are deprecated.
These functions were initially exposed in the public API, but are now considered unnecessary. (#7689)
- The `go.opentelemetry.io/contrib/detectors/aws/ec2` package is deprecated, use `go.opentelemetry.io/contrib/detectors/aws/ec2/v2` instead. (#7725)
- Deprecate direct exposure and usage of mutex objects via structs: introduce equivalent methods for locking/unlocking, and mark mutex fields as deprecated, specially for `go.opentelemetry.io/contrib/samplers/jaegerremote.Sampler` and `go.opentelemetry.io/contrib/propagators/aws/xray.IDGenerator`. (#7726)

### Removed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,22 @@ const miB = 1 << 20
var errorLogger = log.New(log.Writer(), "OTel Lambda Test Error: ", 0)

type mockIDGenerator struct {
sync.Mutex
mu sync.Mutex
traceCount int
spanCount int
}

func (m *mockIDGenerator) NewIDs(context.Context) (trace.TraceID, trace.SpanID) {
m.Lock()
defer m.Unlock()
m.mu.Lock()
defer m.mu.Unlock()
m.traceCount++
m.spanCount++
return [16]byte{byte(m.traceCount)}, [8]byte{byte(m.spanCount)}
}

func (m *mockIDGenerator) NewSpanID(context.Context, trace.TraceID) trace.SpanID {
m.Lock()
defer m.Unlock()
m.mu.Lock()
defer m.mu.Unlock()
m.spanCount++
return [8]byte{byte(m.spanCount)}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type spanKey struct {
}

type monitor struct {
sync.Mutex
mu sync.Mutex
spans map[spanKey]trace.Span
cfg config
semconv semconv.EventMonitor
Expand Down Expand Up @@ -51,9 +51,9 @@ func (m *monitor) Started(ctx context.Context, evt *event.CommandStartedEvent) {
ConnectionID: evt.ConnectionID,
RequestID: evt.RequestID,
}
m.Lock()
m.mu.Lock()
m.spans[key] = span
m.Unlock()
m.mu.Unlock()
}

func (m *monitor) Succeeded(_ context.Context, evt *event.CommandSucceededEvent) {
Expand All @@ -69,12 +69,12 @@ func (m *monitor) Finished(evt *event.CommandFinishedEvent, err error) {
ConnectionID: evt.ConnectionID,
RequestID: evt.RequestID,
}
m.Lock()
m.mu.Lock()
span, ok := m.spans[key]
if ok {
delete(m.spans, key)
}
m.Unlock()
m.mu.Unlock()
if !ok {
return
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type spanKey struct {
}

type monitor struct {
sync.Mutex
mu sync.Mutex
spans map[spanKey]trace.Span
cfg config
}
Expand Down Expand Up @@ -59,9 +59,9 @@ func (m *monitor) Started(ctx context.Context, evt *event.CommandStartedEvent) {
ConnectionID: evt.ConnectionID,
RequestID: evt.RequestID,
}
m.Lock()
m.mu.Lock()
m.spans[key] = span
m.Unlock()
m.mu.Unlock()
}

func (m *monitor) Succeeded(_ context.Context, evt *event.CommandSucceededEvent) {
Expand All @@ -77,12 +77,12 @@ func (m *monitor) Finished(evt *event.CommandFinishedEvent, err error) {
ConnectionID: evt.ConnectionID,
RequestID: evt.RequestID,
}
m.Lock()
m.mu.Lock()
span, ok := m.spans[key]
if ok {
delete(m.spans, key)
}
m.Unlock()
m.mu.Unlock()
if !ok {
return
}
Expand Down
30 changes: 25 additions & 5 deletions propagators/aws/xray/idgenerator.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,36 @@ import (

// IDGenerator is used for generating a new traceID and spanID.
type IDGenerator struct {
sync.Mutex
mu sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

The field needs to be kept as removing an exported field is a breaking change.
The field should be marked as deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't the lign added between L28 and L47 here to handle deprecation ?

Copy link
Member

Choose a reason for hiding this comment

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

randSource *rand.Rand
}

var _ sdktrace.IDGenerator = &IDGenerator{}

// Lock is used to lock the IDGenerator for concurrent use.
// noop method to satisfy the sync.Mutex interface.
//
// Deprecated: mutex is internal and shall not be used.
func (*IDGenerator) Lock() {}

// TryLock attempts to lock the IDGenerator for concurrent use.
// noop method to satisfy the sync.Mutex interface.
//
// Deprecated: mutex is internal and shall not be used.
func (*IDGenerator) TryLock() bool {
return false
}

// Unlock is used to unlock the IDGenerator for concurrent use.
// noop method to satisfy the sync.Mutex interface.
//
// Deprecated: mutex is internal and shall not be used.
func (*IDGenerator) Unlock() {}

// NewSpanID returns a non-zero span ID from a randomly-chosen sequence.
func (gen *IDGenerator) NewSpanID(context.Context, trace.TraceID) trace.SpanID {
gen.Lock()
defer gen.Unlock()
gen.mu.Lock()
defer gen.mu.Unlock()
sid := trace.SpanID{}
_, _ = gen.randSource.Read(sid[:])
return sid
Expand All @@ -40,8 +60,8 @@ func (gen *IDGenerator) NewSpanID(context.Context, trace.TraceID) trace.SpanID {
//
// span ID is from a randomly-chosen sequence.
func (gen *IDGenerator) NewIDs(context.Context) (trace.TraceID, trace.SpanID) {
gen.Lock()
defer gen.Unlock()
gen.mu.Lock()
defer gen.mu.Unlock()

tid := trace.TraceID{}
currentTime := getCurrentTimeHex()
Expand Down
17 changes: 8 additions & 9 deletions samplers/jaegerremote/sampler.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,7 @@ func (*guaranteedThroughputProbabilisticSampler) Description() string {
// perOperationSampler is a delegating sampler that applies guaranteedThroughputProbabilisticSampler
// on a per-operation basis.
type perOperationSampler struct {
sync.RWMutex

mu sync.RWMutex
samplers map[string]*guaranteedThroughputProbabilisticSampler
defaultSampler *probabilisticSampler
lowerBound float64
Expand Down Expand Up @@ -265,15 +264,15 @@ func (s *perOperationSampler) ShouldSample(p trace.SamplingParameters) trace.Sam
}

func (s *perOperationSampler) getSamplerForOperation(operation string) trace.Sampler {
s.RLock()
s.mu.RLock()
sampler, ok := s.samplers[operation]
if ok {
defer s.RUnlock()
defer s.mu.RUnlock()
return sampler
}
s.RUnlock()
s.Lock()
defer s.Unlock()
s.mu.RUnlock()
s.mu.Lock()
defer s.mu.Unlock()

// Check if sampler has already been created
sampler, ok = s.samplers[operation]
Expand All @@ -294,8 +293,8 @@ func (*perOperationSampler) Description() string {
}

func (s *perOperationSampler) update(strategies *jaeger_api_v2.PerOperationSamplingStrategies) {
s.Lock()
defer s.Unlock()
s.mu.Lock()
defer s.mu.Unlock()
newSamplers := map[string]*guaranteedThroughputProbabilisticSampler{}
for _, strategy := range strategies.PerOperationStrategies {
operation := strategy.Operation
Expand Down
34 changes: 27 additions & 7 deletions samplers/jaegerremote/sampler_remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ type Sampler struct {
// Cf. https://github.com/jaegertracing/jaeger-client-go/issues/155, https://pkg.go.dev/sync/atomic#pkg-note-BUG
closed int64 // 0 - not closed, 1 - closed

sync.RWMutex // used to serialize access to samplerConfig.sampler
mu sync.RWMutex // used to serialize access to samplerConfig.sampler
Copy link
Member

Choose a reason for hiding this comment

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

The field needs to be kept as removing an exported field is a breaking change.
The field should be marked as deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't the lign added between L98 and L117 here to handle deprecation ?

Copy link
Member

@pellared pellared Sep 9, 2025

Choose a reason for hiding this comment

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

This only handles deprecation of the "embedded methods".
But after making this field not embedded this would no longer compile:

package main

import "go.opentelemetry.io/contrib/samplers/jaegerremote"

func main() {
	s := new(jaegerremote.Sampler)
	s.RWMutex.Lock()
}

Thus this is a breaking change from Go perspective.
More: open-telemetry/opentelemetry-go#6274

config

serviceName string
Expand All @@ -95,11 +95,31 @@ func New(
return sampler
}

// Lock is used to lock the Sampler for concurrent use.
// noop method to satisfy the sync.RWMutex interface.
//
// Deprecated: mutex is internal and shall not be used.
func (*Sampler) Lock() {}

// TryLock attempts to lock the Sampler for concurrent use.
// noop method to satisfy the sync.RWMutex interface.
//
// Deprecated: mutex is internal and shall not be used.
func (*Sampler) TryLock() bool {
return false
}

// Unlock is used to unlock the Sampler for concurrent use.
// noop method to satisfy the sync.RWMutex interface.
//
// Deprecated: mutex is internal and shall not be used.
func (*Sampler) Unlock() {}

// ShouldSample returns a sampling choice based on the passed sampling
// parameters.
func (s *Sampler) ShouldSample(p trace.SamplingParameters) trace.SamplingResult {
s.RLock()
defer s.RUnlock()
s.mu.RLock()
defer s.mu.RUnlock()
return s.sampler.ShouldSample(p)
}

Expand Down Expand Up @@ -143,8 +163,8 @@ func (s *Sampler) pollControllerWithTicker(ticker *time.Ticker) {
}

func (s *Sampler) setSampler(sampler trace.Sampler) {
s.Lock()
defer s.Unlock()
s.mu.Lock()
defer s.mu.Unlock()
s.sampler = sampler
}

Expand All @@ -162,8 +182,8 @@ func (s *Sampler) UpdateSampler() {
return
}

s.Lock()
defer s.Unlock()
s.mu.Lock()
defer s.mu.Unlock()

if err := s.updateSamplerViaUpdaters(strategy); err != nil {
s.logger.Error(err, "failed to handle sampling strategy response", "response", res)
Expand Down
22 changes: 11 additions & 11 deletions zpages/spanprocessor.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,9 @@ func (ssm *SpanProcessor) spansByLatency(name string, latencyBucketIndex int) []
// It contains sample of spans for error requests (status code is codes.Error);
// and a sample of spans for successful requests, bucketed by latency.
type sampleStore struct {
sync.Mutex // protects everything below.
latency []*bucket
errors *bucket
mu sync.Mutex // protects everything below.
latency []*bucket
errors *bucket
}

// newSampleStore creates a sampleStore.
Expand All @@ -172,8 +172,8 @@ func newSampleStore(latencyBucketSize, errorBucketSize uint) *sampleStore {
}

func (ss *sampleStore) perMethodSummary() *perMethodSummary {
ss.Lock()
defer ss.Unlock()
ss.mu.Lock()
defer ss.mu.Unlock()
p := &perMethodSummary{}
p.errorSpans = ss.errors.len()
for _, b := range ss.latency {
Expand All @@ -183,26 +183,26 @@ func (ss *sampleStore) perMethodSummary() *perMethodSummary {
}

func (ss *sampleStore) spansByLatency(latencyBucketIndex int) []sdktrace.ReadOnlySpan {
ss.Lock()
defer ss.Unlock()
ss.mu.Lock()
defer ss.mu.Unlock()
if latencyBucketIndex < 0 || latencyBucketIndex >= len(ss.latency) {
return nil
}
return ss.latency[latencyBucketIndex].spans()
}

func (ss *sampleStore) errorSpans() []sdktrace.ReadOnlySpan {
ss.Lock()
defer ss.Unlock()
ss.mu.Lock()
defer ss.mu.Unlock()
return ss.errors.spans()
}

// sampleSpan removes adds to the corresponding latency or error bucket.
func (ss *sampleStore) sampleSpan(span sdktrace.ReadOnlySpan) {
code := span.Status().Code

ss.Lock()
defer ss.Unlock()
ss.mu.Lock()
defer ss.mu.Unlock()
if code == codes.Error {
ss.errors.add(span)
return
Expand Down
Loading