Unit tests fail on go-tip

[yurishkuro]

I've seen this fail twice, with the same error. It could indicate a real problem.

https://github.com/jaegertracing/jaeger/actions/runs/11789532893/job/32838429890

--- FAIL: TestCreateCollectorProxy (0.00s)
    --- FAIL: TestCreateCollectorProxy/#02 (0.00s)
panic: Reuse of exported var name: gRPCTarget
	 [recovered]
	panic: Reuse of exported var name: gRPCTarget
	

goroutine 139 [running]:
testing.tRunner.func1.2({0x13a3400, 0xc000220f10})
	/home/runner/sdk/gotip/src/testing/testing.go:1706 +0x3eb
testing.tRunner.func1()
	/home/runner/sdk/gotip/src/testing/testing.go:1709 +0x696
panic({0x13a3400?, 0xc000220f10?})
	/home/runner/sdk/gotip/src/runtime/panic.go:787 +0x132
log.Panicln({0xc00001d258, 0x2, 0x2})
	/home/runner/sdk/gotip/src/log/log.go:446 +0x8e
expvar.Publish({0x152e8a4, 0xa}, {0x16cdc60, 0xc0001f25b0})
	/home/runner/sdk/gotip/src/expvar/expvar.go:311 +0x152
expvar.NewString(...)
	/home/runner/sdk/gotip/src/expvar/expvar.go:347
github.com/jaegertracing/jaeger/cmd/agent/app/reporter.NewConnectMetrics({0x16d8d80, 0xc000222720})
	/home/runner/work/jaeger/jaeger/cmd/agent/app/reporter/connect_metrics.go:32 +0x189
github.com/jaegertracing/jaeger/cmd/agent/app/reporter/grpc.(*ConnBuilder).CreateConnection(0xc0001f6360, {0x16d7ab8, 0xc0003c8000}, 0xc000382180, {0x16d8d80, 0xc0002225a0})
	/home/runner/work/jaeger/jaeger/cmd/agent/app/reporter/grpc/builder.go:99 +0x1025
github.com/jaegertracing/jaeger/cmd/agent/app/reporter/grpc.NewCollectorProxy({0x16d7ab8, 0xc0003c8000}, 0xc0001f6360, 0x0, {0x16d8d80, 0xc0002225a0}, 0xc000382180)
	/home/runner/work/jaeger/jaeger/cmd/agent/app/reporter/grpc/collector_proxy.go:30 +0x9a
github.com/jaegertracing/jaeger/cmd/agent/app.TestCreateCollectorProxy.func1.GRPCCollectorProxyBuilder.1({0x16d7ab8, 0xc0003c8000}, {{{0x1537aeb, 0x4}, 0x0}, 0xc000382180, {0x16d8d80, 0xc0002225a0}})
	/home/runner/work/jaeger/jaeger/cmd/agent/app/proxy_builders.go:15 +0xa5
github.com/jaegertracing/jaeger/cmd/agent/app.CreateCollectorProxy({0x16d7ab8, 0xc0003c8000}, {{{0x1537aeb, 0x4}, 0x0}, 0xc000382180, {0x16d8d80, 0xc0002225a0}}, 0xc00014fd08)
	/home/runner/work/jaeger/jaeger/cmd/agent/app/builder.go:239 +0x1b3
github.com/jaegertracing/jaeger/cmd/agent/app.TestCreateCollectorProxy.func1(0xc00021f500)
	/home/runner/work/jaeger/jaeger/cmd/agent/app/builder_test.go:[249](https://github.com/jaegertracing/jaeger/actions/runs/11789532893/job/32838429890#step:7:250) +0xa45
testing.tRunner(0xc00021f500, 0xc000034fa0)
	/home/runner/sdk/gotip/src/testing/testing.go:1764 +0x226
created by testing.(*T).Run in goroutine 136
	/home/runner/sdk/gotip/src/testing/testing.go:1823 +0x8f3
FAIL	github.com/jaegertracing/jaeger/cmd/agent/app	0.045s

[chahatsagarmain]

@yurishkuro This seems to be due to parallel execution of TestCollectorProxy which conflicts with global state variable gRPCTarget. To keep parallelization, we would need an abstraction to prevent metric name collisions.

[Saumya40-codes]

@yurishkuro This seems to be due to parallel execution of TestCollectorProxy which conflicts with global state variable gRPCTarget. To keep parallelization, we would need an abstraction to prevent metric name collisions.

+1

Test failed when I try to re-run it after multiple itterations because of the concurrent nature as we are using t.Parallel here in cmd/agent/app/builder_test.go in TestCreateCollectorProxy.

Should we considering removing it or using sync.Once or Mutex so that expvar.NewString("gRPCTarget") only happens once?

[yurishkuro]

expvar.NewString("gRPCTarget")

where is this called?

[Saumya40-codes]

expvar.NewString("gRPCTarget")

where is this called?

Inside NewConnectMetrics function in /cmd/agent/app/reporter/connect_metrics.go

Here is that code block:

if r := expvar.Get("gRPCTarget"); r == nil {
	cm.target = expvar.NewString("gRPCTarget")
} else {
	cm.target = r.(*expvar.String)
}

in non parallel nature expvar.NewString("gRPCTarget") should only be happening once as we already keep check before that

[yurishkuro]

ok, good catch. Yes, we should initialize this via Once