Skip to content

Commit cb93051

Browse files
knusbaumknusbaum
authored
ddtrace/tracer: clean up resources in tests (#1643)
Tests have been failing to clean up after themselves and causing memory usage issues during unit test runs. One cause of this is failing to shut down a tracer after starting it, which is solved simply by defering tracer.Close() for those tests. Other tests use newConfig() to generate a tracer config, which they use for various purposes. Unfortunately newConfig() actually creates a statsd client for the new config, which needs to be Close()'d in order to release its resources. It doesn't make much sense to need to Close() a config, or have to Close() something *in* a config, so this problem requires a more complicated solution. Instead, the config still calculates the statsd address, but the actual instantiation of the statsd client is moved into newTracer, where the worker routines and other things that require cleanup are started. This unfortunately requires several other pieces of code to keep a new reference to the statsd client, rather than just to the config as they did previously.
1 parent f7a009a commit cb93051

13 files changed

+163
-60
lines changed

‎ddtrace/tracer/metrics.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func (t *tracer) reportRuntimeMetrics(interval time.Duration) {
4646
runtime.ReadMemStats(&ms)
4747
debug.ReadGCStats(&gc)
4848

49-
statsd := t.config.statsd
49+
statsd := t.statsd
5050
// CPU statistics
5151
statsd.Gauge("runtime.go.num_cpu", float64(runtime.NumCPU()), nil, 1)
5252
statsd.Gauge("runtime.go.num_goroutine", float64(runtime.NumGoroutine()), nil, 1)
@@ -99,9 +99,9 @@ func (t *tracer) reportHealthMetrics(interval time.Duration) {
9999
for {
100100
select {
101101
case <-ticker.C:
102-
t.config.statsd.Count("datadog.tracer.spans_started", int64(atomic.SwapUint32(&t.spansStarted, 0)), nil, 1)
103-
t.config.statsd.Count("datadog.tracer.spans_finished", int64(atomic.SwapUint32(&t.spansFinished, 0)), nil, 1)
104-
t.config.statsd.Count("datadog.tracer.traces_dropped", int64(atomic.SwapUint32(&t.tracesDropped, 0)), []string{"reason:trace_too_large"}, 1)
102+
t.statsd.Count("datadog.tracer.spans_started", int64(atomic.SwapUint32(&t.spansStarted, 0)), nil, 1)
103+
t.statsd.Count("datadog.tracer.spans_finished", int64(atomic.SwapUint32(&t.spansFinished, 0)), nil, 1)
104+
t.statsd.Count("datadog.tracer.traces_dropped", int64(atomic.SwapUint32(&t.tracesDropped, 0)), []string{"reason:trace_too_large"}, 1)
105105
case <-t.stop:
106106
return
107107
}

‎ddtrace/tracer/metrics_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ type testStatsdCall struct {
4848

4949
func withStatsdClient(s statsdClient) StartOption {
5050
return func(c *config) {
51-
c.statsd = s
51+
c.statsdClient = s
5252
}
5353
}
5454

@@ -246,6 +246,7 @@ func (tg *testStatsdClient) Wait(n int, d time.Duration) error {
246246
func TestReportRuntimeMetrics(t *testing.T) {
247247
var tg testStatsdClient
248248
trc := newUnstartedTracer(withStatsdClient(&tg))
249+
defer trc.statsd.Close()
249250

250251
trc.wg.Add(1)
251252
go func() {

‎ddtrace/tracer/option.go

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,9 @@ type config struct {
114114
// combination of the environment variables DD_AGENT_HOST and DD_DOGSTATSD_PORT.
115115
dogstatsdAddr string
116116

117-
// statsd is used for tracking metrics associated with the runtime and the tracer.
118-
statsd statsdClient
117+
// statsdClient is set when a user provides a custom statsd client for tracking metrics
118+
// associated with the runtime and the tracer.
119+
statsdClient statsdClient
119120

120121
// spanRules contains user-defined rules to determine the sampling rate to apply
121122
// to trace spans.
@@ -295,7 +296,7 @@ func newConfig(opts ...StartOption) *config {
295296
log.SetLevel(log.LevelDebug)
296297
}
297298
c.loadAgentFeatures()
298-
if c.statsd == nil {
299+
if c.statsdClient == nil {
299300
// configure statsd client
300301
addr := c.dogstatsdAddr
301302
if addr == "" {
@@ -316,17 +317,23 @@ func newConfig(opts ...StartOption) *config {
316317
// not a valid TCP address, leave it as it is (could be a socket connection)
317318
}
318319
c.dogstatsdAddr = addr
319-
client, err := statsd.New(addr, statsd.WithMaxMessagesPerPayload(40), statsd.WithTags(statsTags(c)))
320-
if err != nil {
321-
log.Warn("Runtime and health metrics disabled: %v", err)
322-
c.statsd = &statsd.NoOpClient{}
323-
} else {
324-
c.statsd = client
325-
}
326320
}
321+
327322
return c
328323
}
329324

325+
func newStatsdClient(c *config) (statsdClient, error) {
326+
if c.statsdClient != nil {
327+
return c.statsdClient, nil
328+
}
329+
330+
client, err := statsd.New(c.dogstatsdAddr, statsd.WithMaxMessagesPerPayload(40), statsd.WithTags(statsTags(c)))
331+
if err != nil {
332+
return &statsd.NoOpClient{}, err
333+
}
334+
return client, nil
335+
}
336+
330337
// defaultHTTPClient returns the default http.Client to start the tracer with.
331338
func defaultHTTPClient() *http.Client {
332339
if _, err := os.Stat(defaultSocketAPM); err == nil {
@@ -476,7 +483,7 @@ func statsTags(c *config) []string {
476483
// withNoopStats is used for testing to disable statsd client
477484
func withNoopStats() StartOption {
478485
return func(c *config) {
479-
c.statsd = &statsd.NoOpClient{}
486+
c.statsdClient = &statsd.NoOpClient{}
480487
}
481488
}
482489

‎ddtrace/tracer/option_test.go

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@ func withTickChan(ch <-chan time.Time) StartOption {
4141
// testStatsd asserts that the given statsd.Client can successfully send metrics
4242
// to a UDP listener located at addr.
4343
func testStatsd(t *testing.T, cfg *config, addr string) {
44-
client := cfg.statsd
44+
client, err := newStatsdClient(cfg)
45+
require.NoError(t, err)
46+
defer client.Close()
4547
require.Equal(t, addr, cfg.dogstatsdAddr)
46-
_, err := net.ResolveUDPAddr("udp", addr)
48+
_, err = net.ResolveUDPAddr("udp", addr)
4749
require.NoError(t, err)
4850

4951
client.Count("name", 1, []string{"tag"}, 1)
@@ -57,7 +59,9 @@ func TestStatsdUDPConnect(t *testing.T) {
5759
cfg := newConfig()
5860
addr := net.JoinHostPort(defaultHostname, "8111")
5961

60-
client := cfg.statsd
62+
client, err := newStatsdClient(cfg)
63+
require.NoError(t, err)
64+
defer client.Close()
6165
require.Equal(t, addr, cfg.dogstatsdAddr)
6266
udpaddr, err := net.ResolveUDPAddr("udp", addr)
6367
require.NoError(t, err)
@@ -118,8 +122,11 @@ func TestAutoDetectStatsd(t *testing.T) {
118122
conn.SetDeadline(time.Now().Add(5 * time.Second))
119123

120124
cfg := newConfig()
125+
statsd, err := newStatsdClient(cfg)
126+
require.NoError(t, err)
127+
defer statsd.Close()
121128
require.Equal(t, cfg.dogstatsdAddr, "unix://"+addr)
122-
cfg.statsd.Count("name", 1, []string{"tag"}, 1)
129+
statsd.Count("name", 1, []string{"tag"}, 1)
123130

124131
buf := make([]byte, 17)
125132
n, err := conn.Read(buf)
@@ -246,11 +253,14 @@ func TestTracerOptionsDefaults(t *testing.T) {
246253
defer globalconfig.SetAnalyticsRate(math.NaN())
247254
assert := assert.New(t)
248255
assert.True(math.IsNaN(globalconfig.AnalyticsRate()))
249-
newTracer(WithAnalyticsRate(0.5))
256+
tracer := newTracer(WithAnalyticsRate(0.5))
257+
defer tracer.Stop()
250258
assert.Equal(0.5, globalconfig.AnalyticsRate())
251-
newTracer(WithAnalytics(false))
259+
tracer = newTracer(WithAnalytics(false))
260+
defer tracer.Stop()
252261
assert.True(math.IsNaN(globalconfig.AnalyticsRate()))
253-
newTracer(WithAnalytics(true))
262+
tracer = newTracer(WithAnalytics(true))
263+
defer tracer.Stop()
254264
assert.Equal(1., globalconfig.AnalyticsRate())
255265
})
256266

@@ -274,6 +284,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
274284
t.Run("dogstatsd", func(t *testing.T) {
275285
t.Run("default", func(t *testing.T) {
276286
tracer := newTracer()
287+
defer tracer.Stop()
277288
c := tracer.config
278289
assert.Equal(t, c.dogstatsdAddr, "localhost:8125")
279290
})
@@ -282,6 +293,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
282293
os.Setenv("DD_AGENT_HOST", "my-host")
283294
defer os.Unsetenv("DD_AGENT_HOST")
284295
tracer := newTracer()
296+
defer tracer.Stop()
285297
c := tracer.config
286298
assert.Equal(t, c.dogstatsdAddr, "my-host:8125")
287299
})
@@ -290,6 +302,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
290302
os.Setenv("DD_DOGSTATSD_PORT", "123")
291303
defer os.Unsetenv("DD_DOGSTATSD_PORT")
292304
tracer := newTracer()
305+
defer tracer.Stop()
293306
c := tracer.config
294307
assert.Equal(t, c.dogstatsdAddr, "localhost:123")
295308
})
@@ -300,6 +313,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
300313
defer os.Unsetenv("DD_AGENT_HOST")
301314
defer os.Unsetenv("DD_DOGSTATSD_PORT")
302315
tracer := newTracer()
316+
defer tracer.Stop()
303317
c := tracer.config
304318
assert.Equal(t, c.dogstatsdAddr, "my-host:123")
305319
})
@@ -308,12 +322,14 @@ func TestTracerOptionsDefaults(t *testing.T) {
308322
os.Setenv("DD_ENV", "testEnv")
309323
defer os.Unsetenv("DD_ENV")
310324
tracer := newTracer()
325+
defer tracer.Stop()
311326
c := tracer.config
312327
assert.Equal(t, "testEnv", c.env)
313328
})
314329

315330
t.Run("option", func(t *testing.T) {
316331
tracer := newTracer(WithDogstatsdAddress("10.1.0.12:4002"))
332+
defer tracer.Stop()
317333
c := tracer.config
318334
assert.Equal(t, c.dogstatsdAddr, "10.1.0.12:4002")
319335
})
@@ -323,6 +339,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
323339
os.Setenv("DD_AGENT_HOST", "trace-agent")
324340
defer os.Unsetenv("DD_AGENT_HOST")
325341
tracer := newTracer()
342+
defer tracer.Stop()
326343
c := tracer.config
327344
assert.Equal(t, "http://trace-agent:8126", c.agentURL)
328345
})
@@ -331,6 +348,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
331348
t.Run("env", func(t *testing.T) {
332349
t.Setenv("DD_TRACE_AGENT_URL", "https://custom:1234")
333350
tracer := newTracer()
351+
defer tracer.Stop()
334352
c := tracer.config
335353
assert.Equal(t, "https://custom:1234", c.agentURL)
336354
})
@@ -340,13 +358,15 @@ func TestTracerOptionsDefaults(t *testing.T) {
340358
t.Setenv("DD_TRACE_AGENT_PORT", "3333")
341359
t.Setenv("DD_TRACE_AGENT_URL", "https://custom:1234")
342360
tracer := newTracer()
361+
defer tracer.Stop()
343362
c := tracer.config
344363
assert.Equal(t, "https://custom:1234", c.agentURL)
345364
})
346365

347366
t.Run("code-override", func(t *testing.T) {
348367
t.Setenv("DD_TRACE_AGENT_URL", "https://custom:1234")
349368
tracer := newTracer(WithAgentAddr("testhost:3333"))
369+
defer tracer.Stop()
350370
c := tracer.config
351371
assert.Equal(t, "http://testhost:3333", c.agentURL)
352372
})
@@ -358,13 +378,15 @@ func TestTracerOptionsDefaults(t *testing.T) {
358378
assert := assert.New(t)
359379
env := "production"
360380
tracer := newTracer(WithEnv(env))
381+
defer tracer.Stop()
361382
c := tracer.config
362383
assert.Equal(env, c.env)
363384
})
364385

365386
t.Run("trace_enabled", func(t *testing.T) {
366387
t.Run("default", func(t *testing.T) {
367388
tracer := newTracer()
389+
defer tracer.Stop()
368390
c := tracer.config
369391
assert.True(t, c.enabled)
370392
})
@@ -373,6 +395,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
373395
os.Setenv("DD_TRACE_ENABLED", "false")
374396
defer os.Unsetenv("DD_TRACE_ENABLED")
375397
tracer := newTracer()
398+
defer tracer.Stop()
376399
c := tracer.config
377400
assert.False(t, c.enabled)
378401
})
@@ -387,6 +410,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
387410
WithDebugMode(true),
388411
WithEnv("testEnv"),
389412
)
413+
defer tracer.Stop()
390414
c := tracer.config
391415
assert.Equal(float64(0.5), c.sampler.(RateSampler).Rate())
392416
assert.Equal("http://ddagent.consul.local:58126", c.agentURL)

‎ddtrace/tracer/sampler_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -643,6 +643,7 @@ func TestRulesSamplerConcurrency(t *testing.T) {
643643
NameRule("notweb.request", 1.0),
644644
}
645645
tracer := newTracer(WithSamplingRules(rules))
646+
defer tracer.Stop()
646647
span := func(wg *sync.WaitGroup) {
647648
defer wg.Done()
648649
tracer.StartSpan("db.query", ServiceName("postgres.db")).Finish()

‎ddtrace/tracer/span_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ func TestSpanFinish(t *testing.T) {
7676
assert := assert.New(t)
7777
wait := time.Millisecond * 2
7878
tracer := newTracer(withTransport(newDefaultTransport()))
79+
defer tracer.Stop()
7980
span := tracer.newRootSpan("pylons.request", "pylons", "/")
8081

8182
// the finish should set finished and the duration
@@ -364,13 +365,15 @@ func TestTraceManualKeepAndManualDrop(t *testing.T) {
364365
} {
365366
t.Run(fmt.Sprintf("%s/local", scenario.tag), func(t *testing.T) {
366367
tracer := newTracer()
368+
defer tracer.Stop()
367369
span := tracer.newRootSpan("root span", "my service", "my resource")
368370
span.SetTag(scenario.tag, true)
369371
assert.Equal(t, scenario.keep, shouldKeep(span))
370372
})
371373

372374
t.Run(fmt.Sprintf("%s/non-local", scenario.tag), func(t *testing.T) {
373375
tracer := newTracer()
376+
defer tracer.Stop()
374377
spanCtx := &spanContext{traceID: 42, spanID: 42}
375378
spanCtx.setSamplingPriority(scenario.p, samplernames.RemoteRate)
376379
span := tracer.StartSpan("non-local root span", ChildOf(spanCtx)).(*span)
@@ -396,6 +399,7 @@ func TestSpanSetDatadogTags(t *testing.T) {
396399
func TestSpanStart(t *testing.T) {
397400
assert := assert.New(t)
398401
tracer := newTracer(withTransport(newDefaultTransport()))
402+
defer tracer.Stop()
399403
span := tracer.newRootSpan("pylons.request", "pylons", "/")
400404

401405
// a new span sets the Start after the initialization
@@ -405,6 +409,7 @@ func TestSpanStart(t *testing.T) {
405409
func TestSpanString(t *testing.T) {
406410
assert := assert.New(t)
407411
tracer := newTracer(withTransport(newDefaultTransport()))
412+
defer tracer.Stop()
408413
span := tracer.newRootSpan("pylons.request", "pylons", "/")
409414
// don't bother checking the contents, just make sure it works.
410415
assert.NotEqual("", span.String())
@@ -463,6 +468,7 @@ func TestSpanSetMetric(t *testing.T) {
463468
t.Run(name, func(t *testing.T) {
464469
assert := assert.New(t)
465470
tracer := newTracer(withTransport(newDefaultTransport()))
471+
defer tracer.Stop()
466472
span := tracer.newRootSpan("http.request", "mux.router", "/")
467473
tt(assert, span)
468474
})
@@ -472,6 +478,7 @@ func TestSpanSetMetric(t *testing.T) {
472478
func TestSpanError(t *testing.T) {
473479
assert := assert.New(t)
474480
tracer := newTracer(withTransport(newDefaultTransport()))
481+
defer tracer.Stop()
475482
span := tracer.newRootSpan("pylons.request", "pylons", "/")
476483

477484
// check the error is set in the default meta
@@ -499,6 +506,7 @@ func TestSpanError(t *testing.T) {
499506
func TestSpanError_Typed(t *testing.T) {
500507
assert := assert.New(t)
501508
tracer := newTracer(withTransport(newDefaultTransport()))
509+
defer tracer.Stop()
502510
span := tracer.newRootSpan("pylons.request", "pylons", "/")
503511

504512
// check the error is set in the default meta
@@ -513,6 +521,7 @@ func TestSpanError_Typed(t *testing.T) {
513521
func TestSpanErrorNil(t *testing.T) {
514522
assert := assert.New(t)
515523
tracer := newTracer(withTransport(newDefaultTransport()))
524+
defer tracer.Stop()
516525
span := tracer.newRootSpan("pylons.request", "pylons", "/")
517526

518527
// don't set the error if it's nil
@@ -574,6 +583,7 @@ func TestSpanModifyWhileFlushing(t *testing.T) {
574583
func TestSpanSamplingPriority(t *testing.T) {
575584
assert := assert.New(t)
576585
tracer := newTracer(withTransport(newDefaultTransport()))
586+
defer tracer.Stop()
577587

578588
span := tracer.newRootSpan("my.name", "my.service", "my.resource")
579589
_, ok := span.Metrics[keySamplingPriority]

‎ddtrace/tracer/stats.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,11 @@ type concentrator struct {
5454
// stopped reports whether the concentrator is stopped (when non-zero)
5555
stopped uint32
5656

57-
wg sync.WaitGroup // waits for any active goroutines
58-
bucketSize int64 // the size of a bucket in nanoseconds
59-
stop chan struct{} // closing this channel triggers shutdown
60-
cfg *config // tracer startup configuration
57+
wg sync.WaitGroup // waits for any active goroutines
58+
bucketSize int64 // the size of a bucket in nanoseconds
59+
stop chan struct{} // closing this channel triggers shutdown
60+
cfg *config // tracer startup configuration
61+
statsdClient statsdClient // statsd client for sending metrics.
6162
}
6263

6364
// newConcentrator creates a new concentrator using the given tracer
@@ -113,10 +114,10 @@ func (c *concentrator) runFlusher(tick <-chan time.Time) {
113114

114115
// statsd returns any tracer configured statsd client, or a no-op.
115116
func (c *concentrator) statsd() statsdClient {
116-
if c.cfg.statsd == nil {
117+
if c.statsdClient == nil {
117118
return &statsd.NoOpClient{}
118119
}
119-
return c.cfg.statsd
120+
return c.statsdClient
120121
}
121122

122123
// runIngester runs the loop which accepts incoming data on the concentrator's In

0 commit comments

Comments
 (0)