Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 12 commits into from
May 31, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
94bace3
Replace internal metrics.Factory usage with direct calls to expvar
prakrit55 May 28, 2024
13afce8
fix
prakrit55 May 29, 2024
7c8b99d
fix
prakrit55 May 29, 2024
1a5728f
fix
prakrit55 May 29, 2024
6d2a1af
fix
prakrit55 May 30, 2024
dfb4a79
fix
prakrit55 May 30, 2024
78299c4
fixing goleak checks
prakrit55 May 30, 2024
66efee2
fix
prakrit55 May 30, 2024
509dfe3
fix
prakrit55 May 30, 2024
9aee328
fix
prakrit55 May 30, 2024
d45db2f
added changes
prakrit55 May 31, 2024
916ce93
lint
prakrit55 May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions cmd/agent/app/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/internal/metrics/fork"
"github.com/jaegertracing/jaeger/internal/metrics/metricsbuilder"
"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/pkg/testutils"
Expand Down Expand Up @@ -103,11 +102,10 @@ func withRunningAgent(t *testing.T, testcase func(string, chan error)) {
logger, logBuf := testutils.NewLogger()
mBldr := &metricsbuilder.Builder{HTTPRoute: "/metrics", Backend: "prometheus"}
metricsFactory, err := mBldr.CreateMetricsFactory("jaeger")
mFactory := fork.New("internal", metrics.NullFactory, metricsFactory)
require.NoError(t, err)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, logger, mFactory)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, logger, metricsFactory)
require.NoError(t, err)
if h := mBldr.Handler(); mFactory != nil && h != nil {
if h := mBldr.Handler(); metricsFactory != nil && h != nil {
logger.Info("Registering metrics handler with HTTP server", zap.String("route", mBldr.HTTPRoute))
agent.GetHTTPRouter().Handle(mBldr.HTTPRoute, h).Methods(http.MethodGet)
}
Expand Down Expand Up @@ -158,9 +156,8 @@ func TestStartStopRace(t *testing.T) {
logger, logBuf := testutils.NewEchoLogger(t)
mBldr := &metricsbuilder.Builder{HTTPRoute: "/metrics", Backend: "prometheus"}
metricsFactory, err := mBldr.CreateMetricsFactory("jaeger")
mFactory := fork.New("internal", metrics.NullFactory, metricsFactory)
require.NoError(t, err)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, logger, mFactory)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, logger, metricsFactory)
require.NoError(t, err)

// This test attempts to hit the data race bug when Stop() is called
Expand Down Expand Up @@ -202,9 +199,8 @@ func TestStartStopWithSocketBufferSet(t *testing.T) {
}
mBldr := &metricsbuilder.Builder{HTTPRoute: "/metrics", Backend: "prometheus"}
metricsFactory, err := mBldr.CreateMetricsFactory("jaeger")
mFactory := fork.New("internal", metrics.NullFactory, metricsFactory)
require.NoError(t, err)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, zap.NewNop(), mFactory)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, zap.NewNop(), metricsFactory)
require.NoError(t, err)

if err := agent.Run(); err != nil {
Expand Down
21 changes: 12 additions & 9 deletions cmd/agent/app/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/jaegertracing/jaeger/cmd/agent/app/reporter"
"github.com/jaegertracing/jaeger/cmd/agent/app/servers"
"github.com/jaegertracing/jaeger/cmd/agent/app/servers/thriftudp"
"github.com/jaegertracing/jaeger/internal/safeexpvar"
"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/ports"
agentThrift "github.com/jaegertracing/jaeger/thrift-gen/agent"
Expand Down Expand Up @@ -111,7 +112,7 @@ func (b *Builder) CreateAgent(primaryProxy CollectorProxy, logger *zap.Logger, m
return nil, fmt.Errorf("cannot create processors: %w", err)
}
server := b.HTTPServer.getHTTPServer(primaryProxy.GetManager(), mFactory, logger)
b.publishOpts(mFactory)
b.publishOpts()

return NewAgent(processors, server, logger), nil
}
Expand All @@ -128,16 +129,18 @@ func (b *Builder) getReporter(primaryProxy CollectorProxy) reporter.Reporter {
return reporter.NewMultiReporter(rep...)
}

func (b *Builder) publishOpts(mFactory metrics.Factory) {
internalFactory := mFactory.Namespace(metrics.NSOptions{Name: "internal"})
func (b *Builder) publishOpts() {
for _, p := range b.Processors {
prefix := fmt.Sprintf(processorPrefixFmt, p.Model, p.Protocol)
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
internalFactory.Gauge(metrics.Options{Name: prefix + suffixServerMaxPacketSize}).
Update(int64(p.Server.MaxPacketSize))
internalFactory.Gauge(metrics.Options{Name: prefix + suffixServerQueueSize}).
Update(int64(p.Server.QueueSize))
internalFactory.Gauge(metrics.Options{Name: prefix + suffixWorkers}).
Update(int64(p.Workers))
safeexpvar.SetExpvarInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// v := expvar.NewInt(prefix + suffixServerMaxPacketSize)
// v.Set(int64(p.Server.MaxPacketSize))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove commented code

safeexpvar.SetExpvarInt(prefix+suffixServerQueueSize, int64(p.Server.QueueSize))
// v = expvar.NewInt(prefix + suffixServerQueueSize)
// v.Set(int64(p.Server.QueueSize))
safeexpvar.SetExpvarInt(prefix+suffixWorkers, int64(p.Workers))
// v = expvar.NewInt(prefix + suffixWorkers)
// v.Set((int64(p.Workers)))
}
}

Expand Down
25 changes: 8 additions & 17 deletions cmd/agent/app/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package app
import (
"context"
"errors"
"expvar"
"flag"
"fmt"
"strings"
"testing"
"time"
Expand All @@ -33,7 +35,6 @@ import (
"github.com/jaegertracing/jaeger/cmd/agent/app/configmanager"
"github.com/jaegertracing/jaeger/cmd/agent/app/reporter"
"github.com/jaegertracing/jaeger/cmd/agent/app/reporter/grpc"
"github.com/jaegertracing/jaeger/internal/metrics/fork"
"github.com/jaegertracing/jaeger/internal/metricstest"
"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/proto-gen/api_v2"
Expand Down Expand Up @@ -308,25 +309,15 @@ func TestPublishOpts(t *testing.T) {

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)
agent, err := cfg.CreateAgent(fakeCollectorProxy{}, zap.NewNop(), baseMetrics)
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",
Value: 4242,
})
forkFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{
Name: "internal.processor.jaeger-binary.server-queue-size",
Value: 24,
})
forkFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{
Name: "internal.processor.jaeger-binary.workers",
Value: 42,
})
p := cfg.Processors[2]
prefix := fmt.Sprintf(processorPrefixFmt, p.Model, p.Protocol)
assert.EqualValues(t, 4242, expvar.Get(prefix+suffixServerMaxPacketSize).(*expvar.Int).Value())
assert.EqualValues(t, 24, expvar.Get(prefix+suffixServerQueueSize).(*expvar.Int).Value())
assert.EqualValues(t, 42, expvar.Get(prefix+suffixWorkers).(*expvar.Int).Value())
}
7 changes: 1 addition & 6 deletions cmd/agent/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ import (
"github.com/jaegertracing/jaeger/cmd/internal/docs"
"github.com/jaegertracing/jaeger/cmd/internal/flags"
"github.com/jaegertracing/jaeger/cmd/internal/status"
"github.com/jaegertracing/jaeger/internal/metrics/expvar"
"github.com/jaegertracing/jaeger/internal/metrics/fork"
"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/pkg/version"
Expand All @@ -58,12 +56,9 @@ func main() {
}
logger := svc.Logger // shortcut

baseFactory := svc.MetricsFactory.
mFactory := svc.MetricsFactory.
Namespace(metrics.NSOptions{Name: "jaeger"}).
Namespace(metrics.NSOptions{Name: "agent"})
mFactory := fork.New("internal",
expvar.NewFactory(), // expvar backend to report settings
baseFactory)
version.NewInfoMetrics(mFactory)

rOpts := new(reporter.Options).InitFromViper(v, logger)
Expand Down
6 changes: 1 addition & 5 deletions cmd/all-in-one/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ import (
"github.com/jaegertracing/jaeger/cmd/internal/status"
queryApp "github.com/jaegertracing/jaeger/cmd/query/app"
"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
"github.com/jaegertracing/jaeger/internal/metrics/expvar"
"github.com/jaegertracing/jaeger/internal/metrics/fork"
"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/jtracer"
"github.com/jaegertracing/jaeger/pkg/metrics"
Expand Down Expand Up @@ -96,9 +94,7 @@ by default uses only in-memory database.`,
return err
}
logger := svc.Logger // shortcut
baseFactory := fork.New("internal",
expvar.NewFactory(), // expvar backend to report settings
svc.MetricsFactory.Namespace(metrics.NSOptions{Name: "jaeger"}))
baseFactory := svc.MetricsFactory.Namespace(metrics.NSOptions{Name: "jaeger"})
version.NewInfoMetrics(baseFactory)
agentMetricsFactory := baseFactory.Namespace(metrics.NSOptions{Name: "agent"})
collectorMetricsFactory := baseFactory.Namespace(metrics.NSOptions{Name: "collector"})
Expand Down
12 changes: 4 additions & 8 deletions cmd/collector/app/collector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (

"github.com/jaegertracing/jaeger/cmd/collector/app/flags"
"github.com/jaegertracing/jaeger/cmd/collector/app/processor"
"github.com/jaegertracing/jaeger/internal/metrics/fork"
"github.com/jaegertracing/jaeger/internal/metricstest"
"github.com/jaegertracing/jaeger/model"
"github.com/jaegertracing/jaeger/pkg/healthcheck"
Expand Down Expand Up @@ -158,11 +157,8 @@ func TestCollector_PublishOpts(t *testing.T) {
// prepare
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)
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
metricsFactory := metricstest.NewFactory(time.Second)
defer metricsFactory.Backend.Stop()
spanWriter := &fakeSpanWriter{}
strategyStore := &mockStrategyStore{}
tm := &tenancy.Manager{}
Expand All @@ -183,11 +179,11 @@ func TestCollector_PublishOpts(t *testing.T) {
require.NoError(t, c.Start(collectorOpts))
defer c.Close()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, there is a metrics emitted from an internal ns

Value: 42,
})
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
6 changes: 1 addition & 5 deletions cmd/collector/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ import (
cmdFlags "github.com/jaegertracing/jaeger/cmd/internal/flags"
"github.com/jaegertracing/jaeger/cmd/internal/printconfig"
"github.com/jaegertracing/jaeger/cmd/internal/status"
"github.com/jaegertracing/jaeger/internal/metrics/expvar"
"github.com/jaegertracing/jaeger/internal/metrics/fork"
"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/pkg/tenancy"
Expand Down Expand Up @@ -73,9 +71,7 @@ func main() {
}
logger := svc.Logger // shortcut
baseFactory := svc.MetricsFactory.Namespace(metrics.NSOptions{Name: "jaeger"})
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
metricsFactory := fork.New("internal",
expvar.NewFactory(), // expvar backend to report settings
baseFactory.Namespace(metrics.NSOptions{Name: "collector"}))
metricsFactory := baseFactory.Namespace(metrics.NSOptions{Name: "collector"})
version.NewInfoMetrics(metricsFactory)
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved

storageFactory.InitFromViper(v, logger)
Expand Down
83 changes: 0 additions & 83 deletions internal/metrics/expvar/cache.go
View file
Open in desktop

This file was deleted.

55 changes: 0 additions & 55 deletions internal/metrics/expvar/cache_test.go
View file
Open in desktop

This file was deleted.

Loading
Loading