Replace internal metrics.Factory usage with direct calls to expvar #5496

prakrit55 · 2024-05-28T07:55:29Z

Which problem is this PR solving?

fixes: Replace "internal" metrics.Factory usage with direct calls to expvar #5495

Description of the changes

removed fork.New factory usage
identify which internal components use "internal" namespace to report settings and replace with expvar directly
removed internal/metrics/fork/*
removed internal/metrics/expvar/*
fixed tests

How was this change tested?

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
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

codecov · 2024-05-28T08:02:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.20%. Comparing base (5900360) to head (916ce93).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5496      +/-   ##
==========================================
+ Coverage   96.00%   96.20%   +0.20%     
==========================================
  Files         330      327       -3     
  Lines       16151    16009     -142     
==========================================
- Hits        15506    15402     -104     
+ Misses        469      434      -35     
+ Partials      176      173       -3

Flag	Coverage Δ
badger_v1	`8.05% <0.00%> (ø)`
badger_v2	`1.93% <0.00%> (ø)`
cassandra-3.x-v1	`16.43% <0.00%> (ø)`
cassandra-3.x-v2	`1.85% <0.00%> (ø)`
cassandra-4.x-v1	`16.43% <0.00%> (ø)`
cassandra-4.x-v2	`1.85% <0.00%> (ø)`
elasticsearch-7.x-v1	`18.88% <0.00%> (-0.02%)`	⬇️
elasticsearch-8.x-v1	`19.07% <0.00%> (-0.02%)`	⬇️
elasticsearch-8.x-v2	`19.08% <0.00%> (?)`
grpc_v1	`9.48% <100.00%> (?)`
grpc_v2	`7.53% <100.00%> (-0.06%)`	⬇️
kafka	`9.78% <0.00%> (ø)`
opensearch-1.x-v1	`18.94% <0.00%> (?)`
opensearch-2.x-v1	`18.92% <0.00%> (?)`
opensearch-2.x-v2	`18.92% <0.00%> (ø)`
unittests	`93.90% <100.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/agent/app/builder.go

yurishkuro · 2024-05-28T16:17:22Z

cmd/agent/app/builder.go

-func (b *Builder) publishOpts(mFactory metrics.Factory) {
-	internalFactory := mFactory.Namespace(metrics.NSOptions{Name: "internal"})
+func (b *Builder) publishOpts() {
+	v := expvar.NewInt("jaeger_agent_max_traces")


there are three independent variables, for each processor

cmd/agent/app/builder.go

yurishkuro · 2024-05-28T16:19:02Z

cmd/agent/app/builder_test.go

 	forkFactory := metricstest.NewFactory(time.Second)
 	defer forkFactory.Stop()


this is not needed

yurishkuro · 2024-05-28T16:19:47Z

cmd/agent/main.go

@@ -61,10 +59,7 @@ func main() {
 			baseFactory := svc.MetricsFactory.


Suggested change

baseFactory := svc.MetricsFactory.

mFactory := svc.MetricsFactory.

this way you don't have to change sites where it's used

cmd/collector/main.go

yurishkuro · 2024-05-28T16:23:06Z

plugin/storage/factory.go

-		Update(int64(f.FactoryConfig.DownsamplingRatio))
-	internalFactory.Gauge(metrics.Options{Name: spanStorageType + "-" + f.SpanReaderType}).
-		Update(1)
+	v := expvar.NewInt("jaeger_storage_max_traces")


why are you calling everything jaeger_storage_max_traces? The names are in the code you're deleting

plugin/storage/factory_test.go

plugin/storage/memory/factory.go

yurishkuro · 2024-05-28T16:25:30Z

plugin/storage/memory/factory_test.go

@@ -80,8 +79,7 @@ func TestPublishOpts(t *testing.T) {
 	defer baseMetrics.Stop()
 	forkFactory := metricstest.NewFactory(time.Second)
 	defer forkFactory.Stop()
-	metricsFactory := fork.New("internal", forkFactory, baseMetrics)
-	require.NoError(t, f.Initialize(metricsFactory, zap.NewNop()))
+	require.NoError(t, f.Initialize(forkFactory, zap.NewNop()))

 	forkFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{


you changed PublishOpt to use expvar, metric factories are no longer relevant. You need to validate that the correct expvar is set (you can see how to do that in expvar/factory_test.go)

thanks for the help

prakrit55 · 2024-05-29T04:06:29Z

its producing panic: Reuse of exported var name: type errors when all file tests are ran at a time

yurishkuro · 2024-05-30T01:20:52Z

cmd/agent/app/builder_test.go

+	if got := expvar.Get(prefix + suffixServerMaxPacketSize).(*expvar.Int).Value(); got != 4242 {
+		t.Errorf(`expected %d, but got %d for %s`, 4242, got, fmt.Sprintf(prefix+suffixServerMaxPacketSize))
+	}


Suggested change

if got := expvar.Get(prefix + suffixServerMaxPacketSize).(*expvar.Int).Value(); got != 4242 {

t.Errorf(`expected %d, but got %d for %s`, 4242, got, fmt.Sprintf(prefix+suffixServerMaxPacketSize))

}

assert.EqualValue(t, 4242, expvar.Get(prefix + suffixServerMaxPacketSize).(*expvar.Int).Value())

cmd/collector/app/collector_test.go

cmd/collector/main.go

yurishkuro · 2024-05-30T01:23:57Z

plugin/storage/factory_test.go

+	if got := expvar.Get(downsamplingRatio).(*expvar.Int).Value(); got != 1 {
+		t.Errorf("expected %d, but got %d for %s", 1, got, downsamplingRatio)
+	}


use assert

yurishkuro · 2024-05-30T01:24:29Z

plugin/storage/memory/factory_test.go

+	if got := expvar.Get("jaeger_storage_memory_max_traces").(*expvar.Int).Value(); got != 100 {
+		t.Errorf("expected %d, but got %d for jaeger_storage_memory_max_traces", 100, got)
+	}


yurishkuro · 2024-05-30T01:30:32Z

its producing panic: Reuse of exported var name: type errors when all file tests are ran at a time

Yeah, I can see why, since tests are running from a single binary and the expvars are global. In the package you deleted we had a cache that was remembering already created expvars by name and on subsequent calls returning the same instance as before. You may need to restore and refactor that cache. It doesn't need to work with metrics API anymore, just with the types of expvars we actually use (mostly int, but maybe others). I would put it under internal/safeexpvar.

Signed-off-by: Griffin <prakritimandal611@gmail.com>

yurishkuro · 2024-05-30T16:31:11Z

cmd/agent/app/builder.go

-			Update(int64(p.Server.QueueSize))
-		internalFactory.Gauge(metrics.Options{Name: prefix + suffixWorkers}).
-			Update(int64(p.Workers))
+		safeexpvar.SetExpvarInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))


Suggested change

safeexpvar.SetExpvarInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))

safeexpvar.SetInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))

it is already explicit from package name that this is expvar

yurishkuro · 2024-05-30T16:31:29Z

cmd/agent/app/builder.go

-			Update(int64(p.Workers))
+		safeexpvar.SetExpvarInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))
+		// v := expvar.NewInt(prefix + suffixServerMaxPacketSize)
+		// v.Set(int64(p.Server.MaxPacketSize))


please remove commented code

yurishkuro · 2024-05-30T16:34:26Z

cmd/collector/app/collector_test.go

 		Name:  "internal.collector.num-workers",
 		Value: 24,
 	})
-	forkFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{
+	metricsFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{
 		Name:  "internal.collector.queue-size",


Where in the code are these emitted? If this test is passing we may have missed a place where these were emitted via "internal" namespace instead of writing directly to expvar

yes, there is a metrics emitted from an internal ns

yurishkuro · 2024-05-30T16:39:44Z

internal/safeexpvar/checkexpvar.go

@@ -1,4 +1,5 @@
-// Copyright (c) 2023 The Jaeger Authors.
+// Copyright (c) 2022 The Jaeger Authors.


Suggested change

// Copyright (c) 2022 The Jaeger Authors.

// Copyright (c) 2024 The Jaeger Authors.

yurishkuro · 2024-05-30T16:42:38Z

internal/safeexpvar/checkexpvar.go

@@ -12,14 +13,16 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-package expvar
+package safeexpvar


internal/safeexpvar/checkexpvar.go -> internal/safeexpvar/safeexpvar.go

yurishkuro · 2024-05-30T16:43:25Z

internal/safeexpvar/cacheexpvar_test.go

+)
+
+func TestSetExpvarInt(t *testing.T) {
+	initialGoroutines := runtime.NumGoroutine()


just add a TestMain like this:

https://github.com/jaegertracing/jaeger/blob/590036037122082b46d830e4c6caa299f7fdbe92/pkg/version/package_test.go#L12_L14

yurishkuro · 2024-05-30T16:45:01Z

plugin/storage/memory/factory_test.go

-	forkFactory := metricstest.NewFactory(time.Second)
-	defer forkFactory.Stop()
-	metricsFactory := fork.New("internal", forkFactory, baseMetrics)
+	metricsFactory := metricstest.NewFactory(time.Second)


you can just use metrics.NullFactory, since you're not validating anything with it

Signed-off-by: Griffin <prakritimandal611@gmail.com>

yurishkuro · 2024-05-30T19:46:07Z

go.mod

+require (
+	golang.org/x/mod v0.17.0 // indirect
+	golang.org/x/tools v0.21.0 // indirect
+)
+


please remove, there are no dependency changes in this PR

and run go mod tidy after removing

yurishkuro · 2024-05-30T19:47:36Z

internal/safeexpvar/safeexpvar_test.go

+// Copyright (c) 2024 The Jaeger Authors.
+// Copyright (c) 2018 Uber Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.


Suggested change

// Copyright (c) 2024 The Jaeger Authors.

// Copyright (c) 2018 Uber Technologies, Inc.

//

// Licensed under the Apache License, Version 2.0 (the "License");

// you may not use this file except in compliance with the License.

// You may obtain a copy of the License at

//

// http://www.apache.org/licenses/LICENSE-2.0

//

// Unless required by applicable law or agreed to in writing, software

// distributed under the License is distributed on an "AS IS" BASIS,

// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

// See the License for the specific language governing permissions and

// limitations under the License.

// Copyright (c) 2024 The Jaeger Authors.

// SPDX-License-Identifier: Apache-2.0

yurishkuro · 2024-05-30T19:47:52Z

internal/safeexpvar/safeexpvar.go

@@ -1,4 +1,5 @@
-// Copyright (c) 2023 The Jaeger Authors.
+// Copyright (c) 2024 The Jaeger Authors.
+// Copyright (c) 2018 Uber Technologies, Inc.


Suggested change

// Copyright (c) 2018 Uber Technologies, Inc.

yurishkuro · 2024-05-30T19:49:05Z

internal/safeexpvar/safeexpvar_test.go

+	expInt, ok := v.(*expvar.Int)
+	assert.True(t, ok, "expected variable %s to be of type *expvar.Int", name)


assert has type checks functions
also this should be require. not assert. since you cannot continue if condition is not met

yurishkuro · 2024-05-30T19:49:39Z

internal/safeexpvar/safeexpvar_test.go

+	assert.NotNil(t, v, "expected variable %s to be created", name)
+	expInt, ok := v.(*expvar.Int)
+	assert.True(t, ok, "expected variable %s to be of type *expvar.Int", name)
+	assert.Equal(t, value, expInt.Value(), "expected variable %s value to be %d", name, value)


Suggested change

assert.Equal(t, value, expInt.Value(), "expected variable %s value to be %d", name, value)

assert.Equal(t, value, expInt.Value())

assert produces descriptive enough message

yurishkuro · 2024-05-30T19:50:34Z

plugin/storage/memory/factory_test.go

+	metricsFactory := metrics.NullFactory
 	require.NoError(t, f.Initialize(metricsFactory, zap.NewNop()))


Suggested change

metricsFactory := metrics.NullFactory

require.NoError(t, f.Initialize(metricsFactory, zap.NewNop()))

require.NoError(t, f.Initialize(metrics.NullFactory, zap.NewNop()))

Signed-off-by: Griffin <prakritimandal611@gmail.com>

yurishkuro

Thanks!

…aegertracing#5496) ## Which problem is this PR solving? - fixes: jaegertracing#5495 ## Description of the changes * removed fork.New factory usage * identify which internal components use "internal" namespace to report settings and replace with expvar directly * removed internal/metrics/fork/* * removed internal/metrics/expvar/* * fixed tests ## How was this change tested? - ## Checklist - [ ] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [ ] 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: Griffin <prakritimandal611@gmail.com> Signed-off-by: Vamshi Maskuri <gwcchintu@gmail.com>

prakrit55 requested a review from a team as a code owner May 28, 2024 07:55

prakrit55 requested a review from albertteoh May 28, 2024 07:55

prakrit55 marked this pull request as draft May 28, 2024 07:56

yurishkuro reviewed May 28, 2024

View reviewed changes

yurishkuro reviewed May 30, 2024

View reviewed changes

prakrit55 added 6 commits May 30, 2024 14:37

Replace internal metrics.Factory usage with direct calls to expvar

94bace3

Signed-off-by: Griffin <prakritimandal611@gmail.com>

fix

13afce8

Signed-off-by: Griffin <prakritimandal611@gmail.com>

fix

7c8b99d

Signed-off-by: Griffin <prakritimandal611@gmail.com>

fix

1a5728f

Signed-off-by: Griffin <prakritimandal611@gmail.com>

fix

6d2a1af

Signed-off-by: Griffin <prakritimandal611@gmail.com>

fix

dfb4a79

Signed-off-by: Griffin <prakritimandal611@gmail.com>

prakrit55 force-pushed the replace-internal-metrics.Factory-expvar branch from 2465cdb to dfb4a79 Compare May 30, 2024 09:07

prakrit55 marked this pull request as ready for review May 30, 2024 09:22

fixing goleak checks

78299c4

Signed-off-by: Griffin <prakritimandal611@gmail.com>

yurishkuro reviewed May 30, 2024

View reviewed changes

yurishkuro added the changelog:bugfix-or-minor-feature label May 30, 2024

yurishkuro reviewed May 30, 2024

View reviewed changes

prakrit55 added 3 commits May 30, 2024 23:01

fix

66efee2

Signed-off-by: Griffin <prakritimandal611@gmail.com>

fix

509dfe3

Signed-off-by: Griffin <prakritimandal611@gmail.com>

fix

9aee328

Signed-off-by: Griffin <prakritimandal611@gmail.com>

yurishkuro reviewed May 30, 2024

View reviewed changes

prakrit55 added 2 commits May 31, 2024 07:10

added changes

d45db2f

Signed-off-by: Griffin <prakritimandal611@gmail.com>

lint

916ce93

Signed-off-by: Griffin <prakritimandal611@gmail.com>

yurishkuro approved these changes May 31, 2024

View reviewed changes

yurishkuro merged commit ac7744e into jaegertracing:main May 31, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace internal metrics.Factory usage with direct calls to expvar #5496

Replace internal metrics.Factory usage with direct calls to expvar #5496

prakrit55 commented May 28, 2024

codecov bot commented May 28, 2024 •

edited

Loading

yurishkuro May 28, 2024

yurishkuro May 28, 2024

yurishkuro May 28, 2024

yurishkuro May 28, 2024

yurishkuro May 28, 2024

prakrit55 May 29, 2024 •

edited

Loading

prakrit55 commented May 29, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro commented May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

prakrit55 May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro May 30, 2024

yurishkuro left a comment

		forkFactory := metricstest.NewFactory(time.Second)
		defer forkFactory.Stop()

		@@ -61,10 +59,7 @@ func main() {
		baseFactory := svc.MetricsFactory.

	baseFactory := svc.MetricsFactory.
	mFactory := svc.MetricsFactory.

	safeexpvar.SetExpvarInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))
	safeexpvar.SetInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))

		@@ -1,4 +1,5 @@
		// Copyright (c) 2023 The Jaeger Authors.
		// Copyright (c) 2022 The Jaeger Authors.

	// Copyright (c) 2022 The Jaeger Authors.
	// Copyright (c) 2024 The Jaeger Authors.

		expInt, ok := v.(*expvar.Int)
		assert.True(t, ok, "expected variable %s to be of type *expvar.Int", name)

	assert.Equal(t, value, expInt.Value(), "expected variable %s value to be %d", name, value)
	assert.Equal(t, value, expInt.Value())

		metricsFactory := metrics.NullFactory
		require.NoError(t, f.Initialize(metricsFactory, zap.NewNop()))

	metricsFactory := metrics.NullFactory
	require.NoError(t, f.Initialize(metricsFactory, zap.NewNop()))
	require.NoError(t, f.Initialize(metrics.NullFactory, zap.NewNop()))

Replace internal metrics.Factory usage with direct calls to expvar #5496

Replace internal metrics.Factory usage with direct calls to expvar #5496

Conversation

prakrit55 commented May 28, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prakrit55 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

prakrit55 commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented May 28, 2024 •

edited

Loading

prakrit55 May 29, 2024 •

edited

Loading