jaegertracing/jaeger

Unit tests fail on go-tip

Closed this issue · 5 comments

yurishkuro commented

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 commented

@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 commented

@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 commented

expvar.NewString("gRPCTarget")

where is this called?

Saumya40-codes commented

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 commented

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

  • 👍1