From b980e87855dcfdd9eedf1b4d8cdc218f60bc986a Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Wed, 13 Jan 2021 17:59:43 -0500 Subject: [PATCH] [refactor] Minor clean-up of #2657 (#2728) * [refactor] Minor clean-up of #2657 Signed-off-by: Yuri Shkuro * add test Signed-off-by: Yuri Shkuro --- cmd/agent/app/reporter/connect_metrics.go | 36 ++++++--- .../app/reporter/connect_metrics_test.go | 73 +++++++------------ cmd/agent/app/reporter/grpc/builder.go | 28 ++----- 3 files changed, 56 insertions(+), 81 deletions(-) diff --git a/cmd/agent/app/reporter/connect_metrics.go b/cmd/agent/app/reporter/connect_metrics.go index 6efe555a6ba..2c416ffbb6f 100644 --- a/cmd/agent/app/reporter/connect_metrics.go +++ b/cmd/agent/app/reporter/connect_metrics.go @@ -15,8 +15,9 @@ package reporter import ( + "expvar" + "github.com/uber/jaeger-lib/metrics" - "go.uber.org/zap" ) type connectMetrics struct { @@ -29,26 +30,37 @@ type connectMetrics struct { // ConnectMetrics include connectMetrics necessary params if want to modify metrics of connectMetrics, must via ConnectMetrics API type ConnectMetrics struct { - Logger *zap.Logger // required - MetricsFactory metrics.Factory // required - connectMetrics *connectMetrics + metrics connectMetrics + target *expvar.String } // NewConnectMetrics will be initialize ConnectMetrics -func (r *ConnectMetrics) NewConnectMetrics() { - r.connectMetrics = new(connectMetrics) - r.MetricsFactory = r.MetricsFactory.Namespace(metrics.NSOptions{Name: "connection_status"}) - metrics.MustInit(r.connectMetrics, r.MetricsFactory, nil) +func NewConnectMetrics(mf metrics.Factory) *ConnectMetrics { + cm := &ConnectMetrics{} + metrics.MustInit(&cm.metrics, mf.Namespace(metrics.NSOptions{Name: "connection_status"}), nil) + + if r := expvar.Get("gRPCTarget"); r == nil { + cm.target = expvar.NewString("gRPCTarget") + } else { + cm.target = r.(*expvar.String) + } + + return cm } // OnConnectionStatusChange used for pass the status parameter when connection is changed // 0 is disconnected, 1 is connected // For quick view status via use `sum(jaeger_agent_connection_status_collector_connected{}) by (instance) > bool 0` -func (r *ConnectMetrics) OnConnectionStatusChange(connected bool) { +func (cm *ConnectMetrics) OnConnectionStatusChange(connected bool) { if connected { - r.connectMetrics.Status.Update(1) - r.connectMetrics.Reconnects.Inc(1) + cm.metrics.Status.Update(1) + cm.metrics.Reconnects.Inc(1) } else { - r.connectMetrics.Status.Update(0) + cm.metrics.Status.Update(0) } } + +// RecordTarget writes the current connection target to an expvar field. +func (cm *ConnectMetrics) RecordTarget(target string) { + cm.target.Set(target) +} diff --git a/cmd/agent/app/reporter/connect_metrics_test.go b/cmd/agent/app/reporter/connect_metrics_test.go index 09bec30987e..d5ff2007f21 100644 --- a/cmd/agent/app/reporter/connect_metrics_test.go +++ b/cmd/agent/app/reporter/connect_metrics_test.go @@ -15,6 +15,7 @@ package reporter import ( + "expvar" "testing" "time" @@ -22,60 +23,38 @@ import ( "github.com/uber/jaeger-lib/metrics/metricstest" ) -type connectMetricsTest struct { - mf *metricstest.Factory -} - -func testConnectMetrics(fn func(tr *connectMetricsTest, r *ConnectMetrics)) { - testConnectMetricsWithParams(&ConnectMetrics{}, fn) -} - -func testConnectMetricsWithParams(cm *ConnectMetrics, fn func(tr *connectMetricsTest, r *ConnectMetrics)) { +func TestConnectMetrics(t *testing.T) { mf := metricstest.NewFactory(time.Hour) - cm.MetricsFactory = mf - cm.NewConnectMetrics() + cm := NewConnectMetrics(mf) - tr := &connectMetricsTest{ - mf: mf, + getGauge := func() map[string]int64 { + _, gauges := mf.Snapshot() + return gauges } - fn(tr, cm) -} - -func testCollectorConnected(r *ConnectMetrics) { - r.OnConnectionStatusChange(true) -} - -func testCollectorAborted(r *ConnectMetrics) { - r.OnConnectionStatusChange(false) -} - -func TestConnectMetrics(t *testing.T) { - - testConnectMetrics(func(tr *connectMetricsTest, r *ConnectMetrics) { - getGauge := func() map[string]int64 { - _, gauges := tr.mf.Snapshot() - return gauges - } + getCount := func() map[string]int64 { + counts, _ := mf.Snapshot() + return counts + } - getCount := func() map[string]int64 { - counts, _ := tr.mf.Snapshot() - return counts - } + // no connection + cm.OnConnectionStatusChange(false) + assert.EqualValues(t, 0, getGauge()["connection_status.collector_connected"]) - // testing connect aborted - testCollectorAborted(r) - assert.EqualValues(t, 0, getGauge()["connection_status.collector_connected"]) + // first connection + cm.OnConnectionStatusChange(true) + assert.EqualValues(t, 1, getGauge()["connection_status.collector_connected"]) + assert.EqualValues(t, 1, getCount()["connection_status.collector_reconnects"]) - // testing connect connected - testCollectorConnected(r) - assert.EqualValues(t, 1, getGauge()["connection_status.collector_connected"]) - assert.EqualValues(t, 1, getCount()["connection_status.collector_reconnects"]) + // reconnect + cm.OnConnectionStatusChange(false) + cm.OnConnectionStatusChange(true) + assert.EqualValues(t, 2, getCount()["connection_status.collector_reconnects"]) - // testing reconnect counts - testCollectorAborted(r) - testCollectorConnected(r) - assert.EqualValues(t, 2, getCount()["connection_status.collector_reconnects"]) + cm.RecordTarget("collector-host") + assert.Equal(t, `"collector-host"`, expvar.Get("gRPCTarget").String()) - }) + // since expvars are singletons, the second constructor should grab the same var + cm2 := NewConnectMetrics(mf) + assert.Same(t, cm.target, cm2.target) } diff --git a/cmd/agent/app/reporter/grpc/builder.go b/cmd/agent/app/reporter/grpc/builder.go index 91dc6171efa..4f220c0a9b9 100644 --- a/cmd/agent/app/reporter/grpc/builder.go +++ b/cmd/agent/app/reporter/grpc/builder.go @@ -17,11 +17,10 @@ package grpc import ( "context" "errors" - "expvar" "fmt" "strings" - "github.com/grpc-ecosystem/go-grpc-middleware/retry" + grpc_retry "github.com/grpc-ecosystem/go-grpc-middleware/retry" "github.com/uber/jaeger-lib/metrics" "go.uber.org/zap" "google.golang.org/grpc" @@ -47,9 +46,6 @@ type ConnBuilder struct { DiscoveryMinPeers int Notifier discovery.Notifier Discoverer discovery.Discoverer - - // for unit test and provide ConnectMetrics and outside call - ConnectMetrics *reporter.ConnectMetrics } // NewConnBuilder creates a new grpc connection builder. @@ -104,30 +100,18 @@ func (b *ConnBuilder) CreateConnection(logger *zap.Logger, mFactory metrics.Fact return nil, err } - if b.ConnectMetrics == nil { - cm := reporter.ConnectMetrics{ - Logger: logger, - MetricsFactory: mFactory.Namespace(metrics.NSOptions{Name: "", Tags: map[string]string{"protocol": "grpc"}}), - } - cm.NewConnectMetrics() - b.ConnectMetrics = &cm - } + connectMetrics := reporter.NewConnectMetrics( + mFactory.Namespace(metrics.NSOptions{Tags: map[string]string{"protocol": "grpc"}}), + ) go func(cc *grpc.ClientConn, cm *reporter.ConnectMetrics) { logger.Info("Checking connection to collector") - var egt *expvar.String - r := expvar.Get("gRPCTarget") - if r == nil { - egt = expvar.NewString("gRPCTarget") - } else { - egt = r.(*expvar.String) - } for { s := cc.GetState() if s == connectivity.Ready { cm.OnConnectionStatusChange(true) - egt.Set(cc.Target()) + cm.RecordTarget(cc.Target()) } else { cm.OnConnectionStatusChange(false) } @@ -135,7 +119,7 @@ func (b *ConnBuilder) CreateConnection(logger *zap.Logger, mFactory metrics.Fact logger.Info("Agent collector connection state change", zap.String("dialTarget", dialTarget), zap.Stringer("status", s)) cc.WaitForStateChange(context.Background(), s) } - }(conn, b.ConnectMetrics) + }(conn, connectMetrics) return conn, nil }