Skip to content

Commit 4ff6794

Browse files
committed
fix: data race on StatsdClientWrapper.delegate in otel metrics client
SetDelegate correctly takes m.mutex, but all delegation methods (Gauge, Count, Histogram, Incr, etc.) read m.delegate without the lock. Switch to sync.RWMutex and take RLock in every delegation method to safely snapshot the delegate before calling on it. Found via a race-detector-enabled build in staging (see #54333 for context).
1 parent 235b290 commit 4ff6794

2 files changed

Lines changed: 113 additions & 20 deletions

File tree

comp/otelcol/otlp/components/metricsclient/metrics_client_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,3 +247,39 @@ func TestNoNilMeter(t *testing.T) {
247247
_, err := InitializeMetricClient(&nilMeterProvider{}, ExporterSourceTag)
248248
assert.ErrorIs(t, err, errNilMeter)
249249
}
250+
251+
func TestStatsdClientWrapperConcurrentSetDelegateAndCount(t *testing.T) {
252+
wrapper := NewStatsdClientWrapper(&statsd.NoOpClient{})
253+
254+
var wg sync.WaitGroup
255+
wg.Add(2)
256+
257+
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
258+
defer cancel()
259+
260+
go func() {
261+
defer wg.Done()
262+
for {
263+
select {
264+
case <-ctx.Done():
265+
return
266+
default:
267+
_ = wrapper.Count("test_count", 1, []string{"otlp:true"}, 1)
268+
}
269+
}
270+
}()
271+
272+
go func() {
273+
defer wg.Done()
274+
for {
275+
select {
276+
case <-ctx.Done():
277+
return
278+
default:
279+
wrapper.SetDelegate(&statsd.NoOpClient{})
280+
}
281+
}
282+
}()
283+
284+
wg.Wait()
285+
}

comp/otelcol/otlp/components/metricsclient/metrics_client_wrapper.go

Lines changed: 77 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
// StatsdClientWrapper is an implementation of ddgostatsd.ClientInterface that delegates all operations to the encompassed ddgostatsd.ClientInterface
1414
type StatsdClientWrapper struct {
1515
delegate ddgostatsd.ClientInterface
16-
mutex sync.Mutex
16+
mutex sync.RWMutex
1717
}
1818

1919
// NewStatsdClientWrapper returns a StatsdClientWrapper
@@ -33,95 +33,152 @@ var _ ddgostatsd.ClientInterface = (*StatsdClientWrapper)(nil)
3333

3434
// Gauge measures the value of a metric at a particular time.
3535
func (m *StatsdClientWrapper) Gauge(name string, value float64, tags []string, rate float64) error {
36-
return m.delegate.Gauge(name, value, tags, rate)
36+
m.mutex.RLock()
37+
delegate := m.delegate
38+
m.mutex.RUnlock()
39+
return delegate.Gauge(name, value, tags, rate)
3740
}
3841

3942
// GaugeWithTimestamp measures the value of a metric at a given time.
4043
func (m *StatsdClientWrapper) GaugeWithTimestamp(name string, value float64, tags []string, rate float64, timestamp time.Time) error {
41-
return m.delegate.GaugeWithTimestamp(name, value, tags, rate, timestamp)
44+
m.mutex.RLock()
45+
delegate := m.delegate
46+
m.mutex.RUnlock()
47+
return delegate.GaugeWithTimestamp(name, value, tags, rate, timestamp)
4248
}
4349

4450
// Count tracks how many times something happened per second.
4551
func (m *StatsdClientWrapper) Count(name string, value int64, tags []string, rate float64) error {
46-
return m.delegate.Count(name, value, tags, rate)
52+
m.mutex.RLock()
53+
delegate := m.delegate
54+
m.mutex.RUnlock()
55+
return delegate.Count(name, value, tags, rate)
4756
}
4857

4958
// CountWithTimestamp tracks how many times something happened at the given second.
5059
func (m *StatsdClientWrapper) CountWithTimestamp(name string, value int64, tags []string, rate float64, timestamp time.Time) error {
51-
return m.delegate.CountWithTimestamp(name, value, tags, rate, timestamp)
60+
m.mutex.RLock()
61+
delegate := m.delegate
62+
m.mutex.RUnlock()
63+
return delegate.CountWithTimestamp(name, value, tags, rate, timestamp)
5264
}
5365

5466
// Histogram tracks the statistical distribution of a set of values on each host.
5567
func (m *StatsdClientWrapper) Histogram(name string, value float64, tags []string, rate float64) error {
56-
return m.delegate.Histogram(name, value, tags, rate)
68+
m.mutex.RLock()
69+
delegate := m.delegate
70+
m.mutex.RUnlock()
71+
return delegate.Histogram(name, value, tags, rate)
5772
}
5873

5974
// Distribution tracks the statistical distribution of a set of values across your infrastructure.
6075
func (m *StatsdClientWrapper) Distribution(name string, value float64, tags []string, rate float64) error {
61-
return m.delegate.Distribution(name, value, tags, rate)
76+
m.mutex.RLock()
77+
delegate := m.delegate
78+
m.mutex.RUnlock()
79+
return delegate.Distribution(name, value, tags, rate)
6280
}
6381

6482
// Decr is just Count of -1
6583
func (m *StatsdClientWrapper) Decr(name string, tags []string, rate float64) error {
66-
return m.delegate.Decr(name, tags, rate)
84+
m.mutex.RLock()
85+
delegate := m.delegate
86+
m.mutex.RUnlock()
87+
return delegate.Decr(name, tags, rate)
6788
}
6889

6990
// Incr is just Count of 1
7091
func (m *StatsdClientWrapper) Incr(name string, tags []string, rate float64) error {
71-
return m.delegate.Incr(name, tags, rate)
92+
m.mutex.RLock()
93+
delegate := m.delegate
94+
m.mutex.RUnlock()
95+
return delegate.Incr(name, tags, rate)
7296
}
7397

7498
// Set counts the number of unique elements in a group.
7599
func (m *StatsdClientWrapper) Set(name string, value string, tags []string, rate float64) error {
76-
return m.delegate.Set(name, value, tags, rate)
100+
m.mutex.RLock()
101+
delegate := m.delegate
102+
m.mutex.RUnlock()
103+
return delegate.Set(name, value, tags, rate)
77104
}
78105

79106
// Timing sends timing information, it is an alias for TimeInMilliseconds
80107
func (m *StatsdClientWrapper) Timing(name string, value time.Duration, tags []string, rate float64) error {
81-
return m.delegate.Timing(name, value, tags, rate)
108+
m.mutex.RLock()
109+
delegate := m.delegate
110+
m.mutex.RUnlock()
111+
return delegate.Timing(name, value, tags, rate)
82112
}
83113

84114
// TimeInMilliseconds sends timing information in milliseconds.
85115
func (m *StatsdClientWrapper) TimeInMilliseconds(name string, value float64, tags []string, rate float64) error {
86-
return m.delegate.TimeInMilliseconds(name, value, tags, rate)
116+
m.mutex.RLock()
117+
delegate := m.delegate
118+
m.mutex.RUnlock()
119+
return delegate.TimeInMilliseconds(name, value, tags, rate)
87120
}
88121

89122
// Event sends the provided Event.
90123
func (m *StatsdClientWrapper) Event(e *ddgostatsd.Event) error {
91-
return m.delegate.Event(e)
124+
m.mutex.RLock()
125+
delegate := m.delegate
126+
m.mutex.RUnlock()
127+
return delegate.Event(e)
92128
}
93129

94130
// SimpleEvent sends an event with the provided title and text.
95131
func (m *StatsdClientWrapper) SimpleEvent(title, text string) error {
96-
return m.delegate.SimpleEvent(title, text)
132+
m.mutex.RLock()
133+
delegate := m.delegate
134+
m.mutex.RUnlock()
135+
return delegate.SimpleEvent(title, text)
97136
}
98137

99138
// ServiceCheck sends the provided ServiceCheck.
100139
func (m *StatsdClientWrapper) ServiceCheck(sc *ddgostatsd.ServiceCheck) error {
101-
return m.delegate.ServiceCheck(sc)
140+
m.mutex.RLock()
141+
delegate := m.delegate
142+
m.mutex.RUnlock()
143+
return delegate.ServiceCheck(sc)
102144
}
103145

104146
// SimpleServiceCheck sends an serviceCheck with the provided name and status.
105147
func (m *StatsdClientWrapper) SimpleServiceCheck(name string, status ddgostatsd.ServiceCheckStatus) error {
106-
return m.delegate.SimpleServiceCheck(name, status)
148+
m.mutex.RLock()
149+
delegate := m.delegate
150+
m.mutex.RUnlock()
151+
return delegate.SimpleServiceCheck(name, status)
107152
}
108153

109154
// Close the client connection.
110155
func (m *StatsdClientWrapper) Close() error {
111-
return m.delegate.Close()
156+
m.mutex.RLock()
157+
delegate := m.delegate
158+
m.mutex.RUnlock()
159+
return delegate.Close()
112160
}
113161

114162
// Flush forces a flush of all the queued dogstatsd payloads.
115163
func (m *StatsdClientWrapper) Flush() error {
116-
return m.delegate.Flush()
164+
m.mutex.RLock()
165+
delegate := m.delegate
166+
m.mutex.RUnlock()
167+
return delegate.Flush()
117168
}
118169

119170
// IsClosed returns if the client has been closed.
120171
func (m *StatsdClientWrapper) IsClosed() bool {
121-
return m.delegate.IsClosed()
172+
m.mutex.RLock()
173+
delegate := m.delegate
174+
m.mutex.RUnlock()
175+
return delegate.IsClosed()
122176
}
123177

124178
// GetTelemetry return the telemetry metrics for the client since it started.
125179
func (m *StatsdClientWrapper) GetTelemetry() ddgostatsd.Telemetry {
126-
return m.delegate.GetTelemetry()
180+
m.mutex.RLock()
181+
delegate := m.delegate
182+
m.mutex.RUnlock()
183+
return delegate.GetTelemetry()
127184
}

0 commit comments

Comments
 (0)