Skip to content

Commit

Permalink
Fix test goroutine leaks in ./cmd/collector/app/ (#5292)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?

Solves part of #5006.

## Description of the changes

I introduced `testutils.VerifyGoLeaks(m)` and then ensured all leaked
goroutines were stopped.

## How was this change tested?

`make lint` and `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [ ] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: Will Sewell <[email protected]>
  • Loading branch information
WillSewell authored Mar 23, 2024
1 parent b8d03f9 commit d887b74
Show file tree
Hide file tree
Showing 7 changed files with 42 additions and 3 deletions.
12 changes: 9 additions & 3 deletions cmd/collector/app/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,15 @@ func (c *Collector) Close() error {
}

// watchers actually never return errors from Close
_ = c.tlsGRPCCertWatcherCloser.Close()
_ = c.tlsHTTPCertWatcherCloser.Close()
_ = c.tlsZipkinCertWatcherCloser.Close()
if c.tlsGRPCCertWatcherCloser != nil {
_ = c.tlsGRPCCertWatcherCloser.Close()
}
if c.tlsHTTPCertWatcherCloser != nil {
_ = c.tlsHTTPCertWatcherCloser.Close()
}
if c.tlsZipkinCertWatcherCloser != nil {
_ = c.tlsZipkinCertWatcherCloser.Close()
}

return nil
}
Expand Down
6 changes: 6 additions & 0 deletions cmd/collector/app/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func TestNewCollector(t *testing.T) {
hc := healthcheck.New()
logger := zap.NewNop()
baseMetrics := metricstest.NewFactory(time.Hour)
defer baseMetrics.Backend.Stop()
spanWriter := &fakeSpanWriter{}
strategyStore := &mockStrategyStore{}
tm := &tenancy.Manager{}
Expand All @@ -78,6 +79,7 @@ func TestCollector_StartErrors(t *testing.T) {
hc := healthcheck.New()
logger := zap.NewNop()
baseMetrics := metricstest.NewFactory(time.Hour)
defer baseMetrics.Backend.Stop()
spanWriter := &fakeSpanWriter{}
strategyStore := &mockStrategyStore{}
tm := &tenancy.Manager{}
Expand All @@ -94,6 +96,7 @@ func TestCollector_StartErrors(t *testing.T) {
err := c.Start(options)
require.Error(t, err)
assert.Contains(t, err.Error(), expErr)
require.NoError(t, c.Close())
})
}

Expand Down Expand Up @@ -131,7 +134,9 @@ func TestCollector_PublishOpts(t *testing.T) {
hc := healthcheck.New()
logger := zap.NewNop()
baseMetrics := metricstest.NewFactory(time.Second)
defer baseMetrics.Backend.Stop()
forkFactory := metricstest.NewFactory(time.Second)
defer forkFactory.Backend.Stop()
metricsFactory := fork.New("internal", forkFactory, baseMetrics)
spanWriter := &fakeSpanWriter{}
strategyStore := &mockStrategyStore{}
Expand Down Expand Up @@ -168,6 +173,7 @@ func TestAggregator(t *testing.T) {
hc := healthcheck.New()
logger := zap.NewNop()
baseMetrics := metricstest.NewFactory(time.Hour)
defer baseMetrics.Backend.Stop()
spanWriter := &fakeSpanWriter{}
strategyStore := &mockStrategyStore{}
agg := &mockAggregator{}
Expand Down
3 changes: 3 additions & 0 deletions cmd/collector/app/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (

func TestProcessorMetrics(t *testing.T) {
baseMetrics := metricstest.NewFactory(time.Hour)
defer baseMetrics.Backend.Stop()
serviceMetrics := baseMetrics.Namespace(jaegerM.NSOptions{Name: "service", Tags: nil})
hostMetrics := baseMetrics.Namespace(jaegerM.NSOptions{Name: "host", Tags: nil})
spm := NewSpanProcessorMetrics(serviceMetrics, hostMetrics, []processor.SpanFormat{processor.SpanFormat("scruffy")})
Expand Down Expand Up @@ -63,6 +64,7 @@ func TestProcessorMetrics(t *testing.T) {

func TestNewTraceCountsBySvc(t *testing.T) {
baseMetrics := metricstest.NewFactory(time.Hour)
defer baseMetrics.Backend.Stop()
metrics := newTraceCountsBySvc(baseMetrics, "not_on_my_level", 3)

metrics.countByServiceName("fry", false, model.SamplerTypeUnrecognized)
Expand Down Expand Up @@ -95,6 +97,7 @@ func TestNewTraceCountsBySvc(t *testing.T) {

func TestNewSpanCountsBySvc(t *testing.T) {
baseMetrics := metricstest.NewFactory(time.Hour)
defer baseMetrics.Backend.Stop()
metrics := newSpanCountsBySvc(baseMetrics, "not_on_my_level", 3)
metrics.countByServiceName("fry", false)
metrics.countByServiceName("leela", false)
Expand Down
14 changes: 14 additions & 0 deletions cmd/collector/app/package_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) 2024 The Jaeger Authors.
// SPDX-License-Identifier: Apache-2.0

package app

import (
"testing"

"github.com/jaegertracing/jaeger/pkg/testutils"
)

func TestMain(m *testing.M) {
testutils.VerifyGoLeaks(m)
}
1 change: 1 addition & 0 deletions cmd/collector/app/span_handler_builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func TestNewSpanHandlerBuilder(t *testing.T) {
assert.NotNil(t, spanHandlers.JaegerBatchesHandler)
assert.NotNil(t, spanHandlers.GRPCHandler)
assert.NotNil(t, spanProcessor)
require.NoError(t, spanProcessor.Close())
}

func TestDefaultSpanFilter(t *testing.T) {
Expand Down
5 changes: 5 additions & 0 deletions cmd/collector/app/span_processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func TestBySvcMetrics(t *testing.T) {

for _, test := range tests {
mb := metricstest.NewFactory(time.Hour)
defer mb.Backend.Stop()
logger := zap.NewNop()
serviceMetrics := mb.Namespace(metrics.NSOptions{Name: "service", Tags: nil})
hostMetrics := mb.Namespace(metrics.NSOptions{Name: "host", Tags: nil})
Expand Down Expand Up @@ -258,6 +259,7 @@ func TestSpanProcessorErrors(t *testing.T) {
err: fmt.Errorf("some-error"),
}
mb := metricstest.NewFactory(time.Hour)
defer mb.Backend.Stop()
serviceMetrics := mb.Namespace(metrics.NSOptions{Name: "service", Tags: nil})
p := NewSpanProcessor(w,
nil,
Expand Down Expand Up @@ -342,6 +344,7 @@ func TestSpanProcessorBusy(t *testing.T) {

func TestSpanProcessorWithNilProcess(t *testing.T) {
mb := metricstest.NewFactory(time.Hour)
defer mb.Backend.Stop()
serviceMetrics := mb.Namespace(metrics.NSOptions{Name: "service", Tags: nil})

w := &fakeSpanWriter{}
Expand Down Expand Up @@ -440,6 +443,7 @@ func TestSpanProcessorCountSpan(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
mb := metricstest.NewFactory(time.Hour)
defer mb.Backend.Stop()
m := mb.Namespace(metrics.NSOptions{})

w := &fakeSpanWriter{}
Expand Down Expand Up @@ -606,6 +610,7 @@ func TestStartDynQueueSizeUpdater(t *testing.T) {
}

assert.EqualValues(t, 104857, p.queue.Capacity())
require.NoError(t, p.Close())
}

func TestAdditionalProcessors(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions internal/metricstest/local.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,3 +392,7 @@ func (l *Factory) Namespace(scope metrics.NSOptions) metrics.Factory {
Backend: l.Backend,
}
}

func (l *Factory) Stop() {
l.Backend.Stop()
}

0 comments on commit d887b74

Please sign in to comment.