Skip to content

Commit

Permalink
Fix leaks in cmd/agent/app (#5101)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?
- Part of #5006 

## Description of the changes
- Added TestMain to package folders
- Added make goleak into unit test ci

## How was this change tested?
- please run make goleak 

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

---------

Signed-off-by: Aleem Isiaka <[email protected]>
  • Loading branch information
limistah authored Jan 18, 2024
1 parent 26579f0 commit 9997cce
Show file tree
Hide file tree
Showing 7 changed files with 70 additions and 40 deletions.
1 change: 1 addition & 0 deletions cmd/agent/app/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ func withRunningAgent(t *testing.T, testcase func(string, chan error)) {
HostPort: "127.0.0.1:0",
},
}

logger, logBuf := testutils.NewLogger()
mBldr := &metricsbuilder.Builder{HTTPRoute: "/metrics", Backend: "prometheus"}
metricsFactory, err := mBldr.CreateMetricsFactory("jaeger")
Expand Down
6 changes: 4 additions & 2 deletions cmd/agent/app/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package app

import (
"context"
"fmt"
"io"
"net/http"
Expand Down Expand Up @@ -237,16 +238,17 @@ type ProxyBuilderOptions struct {
}

// CollectorProxyBuilder is a func which builds CollectorProxy.
type CollectorProxyBuilder func(ProxyBuilderOptions) (CollectorProxy, error)
type CollectorProxyBuilder func(context.Context, ProxyBuilderOptions) (CollectorProxy, error)

// CreateCollectorProxy creates collector proxy
func CreateCollectorProxy(
ctx context.Context,
opts ProxyBuilderOptions,
builders map[reporter.Type]CollectorProxyBuilder,
) (CollectorProxy, error) {
builder, ok := builders[opts.ReporterType]
if !ok {
return nil, fmt.Errorf("unknown reporter type %s", string(opts.ReporterType))
}
return builder(opts)
return builder(ctx, opts)
}
77 changes: 43 additions & 34 deletions cmd/agent/app/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"context"
"errors"
"flag"
"fmt"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -176,7 +175,6 @@ func TestMultipleCollectorProxies(t *testing.T) {
r := b.getReporter(rb)
mr, ok := r.(reporter.MultiReporter)
require.True(t, ok)
fmt.Println(mr)
assert.Equal(t, rb, mr[0])
assert.Equal(t, ra, mr[1])
}
Expand Down Expand Up @@ -235,50 +233,57 @@ func TestCreateCollectorProxy(t *testing.T) {
}

for _, test := range tests {
flags := &flag.FlagSet{}
grpc.AddFlags(flags)
reporter.AddFlags(flags)
t.Run("", func(t *testing.T) {
flags := &flag.FlagSet{}
grpc.AddFlags(flags)
reporter.AddFlags(flags)

command := cobra.Command{}
command.PersistentFlags().AddGoFlagSet(flags)
v := viper.New()
v.BindPFlags(command.PersistentFlags())
command := cobra.Command{}
command.PersistentFlags().AddGoFlagSet(flags)
v := viper.New()
v.BindPFlags(command.PersistentFlags())

err := command.ParseFlags(test.flags)
require.NoError(t, err)

rOpts := new(reporter.Options).InitFromViper(v, zap.NewNop())
grpcBuilder, err := grpc.NewConnBuilder().InitFromViper(v)
require.NoError(t, err)

metricsFactory := metricstest.NewFactory(time.Microsecond)
err := command.ParseFlags(test.flags)
require.NoError(t, err)

builders := map[reporter.Type]CollectorProxyBuilder{
reporter.GRPC: GRPCCollectorProxyBuilder(grpcBuilder),
}
proxy, err := CreateCollectorProxy(ProxyBuilderOptions{
Options: *rOpts,
Metrics: metricsFactory,
Logger: zap.NewNop(),
}, builders)
if test.err != "" {
require.EqualError(t, err, test.err)
assert.Nil(t, proxy)
} else {
rOpts := new(reporter.Options).InitFromViper(v, zap.NewNop())
grpcBuilder, err := grpc.NewConnBuilder().InitFromViper(v)
require.NoError(t, err)
proxy.GetReporter().EmitBatch(context.Background(), jaeger.NewBatch())
metricsFactory.AssertCounterMetrics(t, test.metric)
}
metricsFactory := metricstest.NewFactory(time.Microsecond)
defer metricsFactory.Stop()
builders := map[reporter.Type]CollectorProxyBuilder{
reporter.GRPC: GRPCCollectorProxyBuilder(grpcBuilder),
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
proxy, err := CreateCollectorProxy(ctx, ProxyBuilderOptions{
Options: *rOpts,
Metrics: metricsFactory,
Logger: zap.NewNop(),
}, builders)
if err == nil {
defer proxy.Close()
}
if test.err != "" {
require.EqualError(t, err, test.err)
assert.Nil(t, proxy)
} else {
require.NoError(t, err)
proxy.GetReporter().EmitBatch(context.Background(), jaeger.NewBatch())
metricsFactory.AssertCounterMetrics(t, test.metric)
}
})
}
}

func TestCreateCollectorProxy_UnknownReporter(t *testing.T) {
grpcBuilder := grpc.NewConnBuilder()

builders := map[reporter.Type]CollectorProxyBuilder{
reporter.GRPC: GRPCCollectorProxyBuilder(grpcBuilder),
}
proxy, err := CreateCollectorProxy(ProxyBuilderOptions{}, builders)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
proxy, err := CreateCollectorProxy(ctx, ProxyBuilderOptions{}, builders)
assert.Nil(t, proxy)
require.EqualError(t, err, "unknown reporter type ")
}
Expand All @@ -302,11 +307,15 @@ func TestPublishOpts(t *testing.T) {
cfg.InitFromViper(v)

baseMetrics := metricstest.NewFactory(time.Second)
defer baseMetrics.Stop()
forkFactory := metricstest.NewFactory(time.Second)
defer forkFactory.Stop()
metricsFactory := fork.New("internal", forkFactory, baseMetrics)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, zap.NewNop(), metricsFactory)
require.NoError(t, err)
assert.NotNil(t, agent)
require.NoError(t, agent.Run())
defer agent.Stop()

forkFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{
Name: "internal.processor.jaeger-binary.server-max-packet-size",
Expand Down
14 changes: 14 additions & 0 deletions cmd/agent/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)
}
3 changes: 1 addition & 2 deletions cmd/agent/app/proxy_builders.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ import (

// GRPCCollectorProxyBuilder creates CollectorProxyBuilder for GRPC reporter
func GRPCCollectorProxyBuilder(builder *grpc.ConnBuilder) CollectorProxyBuilder {
return func(opts ProxyBuilderOptions) (proxy CollectorProxy, err error) {
ctx := context.Background()
return func(ctx context.Context, opts ProxyBuilderOptions) (proxy CollectorProxy, err error) {
return grpc.NewCollectorProxy(ctx, builder, opts.AgentTags, opts.Metrics, opts.Logger)
}
}
5 changes: 4 additions & 1 deletion cmd/agent/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package main

import (
"context"
"fmt"
"os"

Expand Down Expand Up @@ -73,7 +74,9 @@ func main() {
builders := map[reporter.Type]app.CollectorProxyBuilder{
reporter.GRPC: app.GRPCCollectorProxyBuilder(grpcBuilder),
}
cp, err := app.CreateCollectorProxy(app.ProxyBuilderOptions{
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
cp, err := app.CreateCollectorProxy(ctx, app.ProxyBuilderOptions{
Options: *rOpts,
Logger: logger,
Metrics: mFactory,
Expand Down
4 changes: 3 additions & 1 deletion cmd/all-in-one/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,12 @@ by default uses only in-memory database.`,
if len(grpcBuilder.CollectorHostPorts) == 0 {
grpcBuilder.CollectorHostPorts = append(grpcBuilder.CollectorHostPorts, cOpts.GRPC.HostPort)
}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
builders := map[agentRep.Type]agentApp.CollectorProxyBuilder{
agentRep.GRPC: agentApp.GRPCCollectorProxyBuilder(grpcBuilder),
}
cp, err := agentApp.CreateCollectorProxy(agentApp.ProxyBuilderOptions{
cp, err := agentApp.CreateCollectorProxy(ctx, agentApp.ProxyBuilderOptions{
Options: *repOpts,
Logger: logger,
Metrics: agentMetricsFactory,
Expand Down

0 comments on commit 9997cce

Please sign in to comment.