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 all 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
15 changes: 6 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,12 @@ 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.SetInt(prefix+suffixServerMaxPacketSize, int64(p.Server.MaxPacketSize))
safeexpvar.SetInt(prefix+suffixServerQueueSize, int64(p.Server.QueueSize))
safeexpvar.SetInt(prefix+suffixWorkers, 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
6 changes: 3 additions & 3 deletions cmd/collector/app/collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/jaegertracing/jaeger/cmd/collector/app/processor"
"github.com/jaegertracing/jaeger/cmd/collector/app/sampling/strategystore"
"github.com/jaegertracing/jaeger/cmd/collector/app/server"
"github.com/jaegertracing/jaeger/internal/safeexpvar"
"github.com/jaegertracing/jaeger/model"
"github.com/jaegertracing/jaeger/pkg/healthcheck"
"github.com/jaegertracing/jaeger/pkg/metrics"
Expand Down Expand Up @@ -170,9 +171,8 @@ func (c *Collector) Start(options *flags.CollectorOptions) error {
}

func (c *Collector) publishOpts(cOpts *flags.CollectorOptions) {
internalFactory := c.metricsFactory.Namespace(metrics.NSOptions{Name: "internal"})
internalFactory.Gauge(metrics.Options{Name: metricNumWorkers}).Update(int64(cOpts.NumWorkers))
internalFactory.Gauge(metrics.Options{Name: metricQueueSize}).Update(int64(cOpts.QueueSize))
safeexpvar.SetInt(metricNumWorkers, int64(cOpts.NumWorkers))
safeexpvar.SetInt(metricQueueSize, int64(cOpts.QueueSize))
}

// Close the component and all its underlying dependencies
Expand Down
21 changes: 6 additions & 15 deletions cmd/collector/app/collector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package app

import (
"context"
"expvar"
"io"
"sync/atomic"
"testing"
Expand All @@ -27,7 +28,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 +158,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 @@ -182,15 +179,9 @@ func TestCollector_PublishOpts(t *testing.T) {

require.NoError(t, c.Start(collectorOpts))
defer c.Close()

forkFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{
Name: "internal.collector.num-workers",
Value: 24,
})
forkFactory.AssertGaugeMetrics(t, metricstest.ExpectedMetric{
Name: "internal.collector.queue-size",
Value: 42,
})
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
c.publishOpts(collectorOpts)
assert.EqualValues(t, 24, expvar.Get(metricNumWorkers).(*expvar.Int).Value())
assert.EqualValues(t, 42, expvar.Get(metricQueueSize).(*expvar.Int).Value())
}

func TestAggregator(t *testing.T) {
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.

Loading
Loading