From 7f1085dda9d54abcbb6d89ba29961bb38b38674b Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Wed, 24 Jun 2020 17:52:34 -0400 Subject: [PATCH 01/25] Add resolved udp connection type, continually resolve dns names in background Signed-off-by: Trevor Foster --- Gopkg.lock | 75 +++++++++++++- config/config.go | 6 +- config/config_test.go | 8 +- reporter_test.go | 2 +- testutils/mock_agent.go | 3 +- transport_udp.go | 5 +- transport_udp_test.go | 9 +- utils/resolved_udp_conn.go | 169 ++++++++++++++++++++++++++++++++ utils/resolved_udp_conn_test.go | 168 +++++++++++++++++++++++++++++++ utils/udp_client.go | 41 ++++++-- 10 files changed, 463 insertions(+), 23 deletions(-) create mode 100644 utils/resolved_udp_conn.go create mode 100644 utils/resolved_udp_conn_test.go diff --git a/Gopkg.lock b/Gopkg.lock index 2a5215a5..387958b1 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -142,10 +142,19 @@ version = "v0.0.5" [[projects]] - digest = "1:0496f0e99014b7fd0a560c539f51d0882731137b85494142f47e550e4657176a" + digest = "1:ac83cf90d08b63ad5f7e020ef480d319ae890c208f8524622a2f3136e2686b02" + name = "github.com/stretchr/objx" + packages = ["."] + pruneopts = "UT" + revision = "477a77ecc69700c7cdeb1fa9e129548e1c1c393c" + version = "v0.1.1" + +[[projects]] + digest = "1:d88ba57c4e8f5db6ce9ab6605a89f4542ee751b576884ba5271c2ba3d4b6f2d2" name = "github.com/stretchr/testify" packages = [ "assert", + "mock", "require", "suite", ] @@ -153,6 +162,42 @@ revision = "221dbe5ed46703ee255b1da0dec05086f5035f62" version = "v1.4.0" +[[projects]] + digest = "1:5b98956718573850caf7e0fd00b571a6657c4ef1f345ddf0c96b43ce355fe862" + name = "github.com/uber/jaeger-client-go" + packages = [ + ".", + "config", + "crossdock/client", + "crossdock/common", + "crossdock/endtoend", + "crossdock/log", + "crossdock/server", + "crossdock/thrift/tracetest", + "internal/baggage", + "internal/baggage/remote", + "internal/reporterstats", + "internal/spanlog", + "internal/throttler", + "internal/throttler/remote", + "log", + "log/zap/mock_opentracing", + "rpcmetrics", + "testutils", + "thrift", + "thrift-gen/agent", + "thrift-gen/baggage", + "thrift-gen/jaeger", + "thrift-gen/sampling", + "thrift-gen/zipkincore", + "transport", + "transport/zipkin", + "utils", + ] + pruneopts = "UT" + revision = "66c008c3d6ad856cac92a0af53186efbffa8e6a5" + version = "v2.24.0" + [[projects]] digest = "1:0ec60ffd594af00ba1660bc746aa0e443d27dd4003dee55f9d08a0b4ff5431a3" name = "github.com/uber/jaeger-lib" @@ -314,8 +359,36 @@ "github.com/pkg/errors", "github.com/prometheus/client_golang/prometheus", "github.com/stretchr/testify/assert", + "github.com/stretchr/testify/mock", "github.com/stretchr/testify/require", "github.com/stretchr/testify/suite", + "github.com/uber/jaeger-client-go", + "github.com/uber/jaeger-client-go/config", + "github.com/uber/jaeger-client-go/crossdock/client", + "github.com/uber/jaeger-client-go/crossdock/common", + "github.com/uber/jaeger-client-go/crossdock/endtoend", + "github.com/uber/jaeger-client-go/crossdock/log", + "github.com/uber/jaeger-client-go/crossdock/server", + "github.com/uber/jaeger-client-go/crossdock/thrift/tracetest", + "github.com/uber/jaeger-client-go/internal/baggage", + "github.com/uber/jaeger-client-go/internal/baggage/remote", + "github.com/uber/jaeger-client-go/internal/reporterstats", + "github.com/uber/jaeger-client-go/internal/spanlog", + "github.com/uber/jaeger-client-go/internal/throttler", + "github.com/uber/jaeger-client-go/internal/throttler/remote", + "github.com/uber/jaeger-client-go/log", + "github.com/uber/jaeger-client-go/log/zap/mock_opentracing", + "github.com/uber/jaeger-client-go/rpcmetrics", + "github.com/uber/jaeger-client-go/testutils", + "github.com/uber/jaeger-client-go/thrift", + "github.com/uber/jaeger-client-go/thrift-gen/agent", + "github.com/uber/jaeger-client-go/thrift-gen/baggage", + "github.com/uber/jaeger-client-go/thrift-gen/jaeger", + "github.com/uber/jaeger-client-go/thrift-gen/sampling", + "github.com/uber/jaeger-client-go/thrift-gen/zipkincore", + "github.com/uber/jaeger-client-go/transport", + "github.com/uber/jaeger-client-go/transport/zipkin", + "github.com/uber/jaeger-client-go/utils", "github.com/uber/jaeger-lib/metrics", "github.com/uber/jaeger-lib/metrics/metricstest", "github.com/uber/jaeger-lib/metrics/prometheus", diff --git a/config/config.go b/config/config.go index e6ffb987..b2c49dbe 100644 --- a/config/config.go +++ b/config/config.go @@ -384,7 +384,7 @@ func (rc *ReporterConfig) NewReporter( metrics *jaeger.Metrics, logger jaeger.Logger, ) (jaeger.Reporter, error) { - sender, err := rc.newTransport() + sender, err := rc.newTransport(logger) if err != nil { return nil, err } @@ -401,7 +401,7 @@ func (rc *ReporterConfig) NewReporter( return reporter, err } -func (rc *ReporterConfig) newTransport() (jaeger.Transport, error) { +func (rc *ReporterConfig) newTransport(logger jaeger.Logger) (jaeger.Transport, error) { switch { case rc.CollectorEndpoint != "": httpOptions := []transport.HTTPOption{transport.HTTPBatchSize(1), transport.HTTPHeaders(rc.HTTPHeaders)} @@ -410,6 +410,6 @@ func (rc *ReporterConfig) newTransport() (jaeger.Transport, error) { } return transport.NewHTTPTransport(rc.CollectorEndpoint, httpOptions...), nil default: - return jaeger.NewUDPTransport(rc.LocalAgentHostPort, 0) + return jaeger.NewUDPTransport(rc.LocalAgentHostPort, 0, logger) } } diff --git a/config/config_test.go b/config/config_test.go index c273d5c8..ad67df5f 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -560,8 +560,8 @@ func TestInvalidSamplerType(t *testing.T) { func TestUDPTransportType(t *testing.T) { rc := &ReporterConfig{LocalAgentHostPort: "localhost:1234"} - expect, _ := jaeger.NewUDPTransport(rc.LocalAgentHostPort, 0) - sender, err := rc.newTransport() + expect, _ := jaeger.NewUDPTransport(rc.LocalAgentHostPort, 0, log.NullLogger) + sender, err := rc.newTransport(log.NullLogger) require.NoError(t, err) require.IsType(t, expect, sender) } @@ -569,7 +569,7 @@ func TestUDPTransportType(t *testing.T) { func TestHTTPTransportType(t *testing.T) { rc := &ReporterConfig{CollectorEndpoint: "http://1.2.3.4:5678/api/traces"} expect := transport.NewHTTPTransport(rc.CollectorEndpoint) - sender, err := rc.newTransport() + sender, err := rc.newTransport(log.NullLogger) require.NoError(t, err) require.IsType(t, expect, sender) } @@ -581,7 +581,7 @@ func TestHTTPTransportTypeWithAuth(t *testing.T) { Password: "auth_pass", } expect := transport.NewHTTPTransport(rc.CollectorEndpoint) - sender, err := rc.newTransport() + sender, err := rc.newTransport(log.NullLogger) require.NoError(t, err) require.IsType(t, expect, sender) } diff --git a/reporter_test.go b/reporter_test.go index 492e9cea..64166244 100644 --- a/reporter_test.go +++ b/reporter_test.go @@ -213,7 +213,7 @@ func TestUDPReporter(t *testing.T) { testRemoteReporterWithSender(t, func(m *Metrics) (Transport, error) { - return NewUDPTransport(agent.SpanServerAddr(), 0) + return NewUDPTransport(agent.SpanServerAddr(), 0, log.NullLogger) }, func() []*j.Batch { return agent.GetJaegerBatches() diff --git a/testutils/mock_agent.go b/testutils/mock_agent.go index dd5e0c0d..1ee32320 100644 --- a/testutils/mock_agent.go +++ b/testutils/mock_agent.go @@ -23,6 +23,7 @@ import ( "sync" "sync/atomic" + "github.com/uber/jaeger-client-go/log" "github.com/uber/jaeger-client-go/thrift" "github.com/uber/jaeger-client-go/thrift-gen/agent" @@ -82,7 +83,7 @@ func (s *MockAgent) SpanServerAddr() string { // SpanServerClient returns a UDP client that can be used to send spans to the MockAgent func (s *MockAgent) SpanServerClient() (agent.Agent, error) { - return utils.NewAgentClientUDP(s.SpanServerAddr(), 0) + return utils.NewAgentClientUDP(s.SpanServerAddr(), 0, log.NullLogger) } // SamplingServerAddr returns the host:port of HTTP server exposing sampling strategy endpoint diff --git a/transport_udp.go b/transport_udp.go index 7370d800..309dbdf0 100644 --- a/transport_udp.go +++ b/transport_udp.go @@ -19,6 +19,7 @@ import ( "fmt" "github.com/uber/jaeger-client-go/internal/reporterstats" + "github.com/uber/jaeger-client-go/log" "github.com/uber/jaeger-client-go/thrift" j "github.com/uber/jaeger-client-go/thrift-gen/jaeger" "github.com/uber/jaeger-client-go/utils" @@ -59,7 +60,7 @@ type udpSender struct { // NewUDPTransport creates a reporter that submits spans to jaeger-agent. // TODO: (breaking change) move to transport/ package. -func NewUDPTransport(hostPort string, maxPacketSize int) (Transport, error) { +func NewUDPTransport(hostPort string, maxPacketSize int, logger log.Logger) (Transport, error) { if len(hostPort) == 0 { hostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) } @@ -73,7 +74,7 @@ func NewUDPTransport(hostPort string, maxPacketSize int) (Transport, error) { thriftBuffer := thrift.NewTMemoryBufferLen(maxPacketSize) thriftProtocol := protocolFactory.GetProtocol(thriftBuffer) - client, err := utils.NewAgentClientUDP(hostPort, maxPacketSize) + client, err := utils.NewAgentClientUDP(hostPort, maxPacketSize, logger) if err != nil { return nil, err } diff --git a/transport_udp_test.go b/transport_udp_test.go index a0c55894..e5f14572 100644 --- a/transport_udp_test.go +++ b/transport_udp_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/jaeger-client-go/log" "github.com/uber/jaeger-client-go/internal/reporterstats" "github.com/uber/jaeger-client-go/testutils" @@ -125,7 +126,7 @@ func TestUDPSenderFlush(t *testing.T) { spanSize := getThriftSpanByteLength(t, span) processSize := getThriftProcessByteLengthFromTracer(t, jaegerTracer) - sender, err := NewUDPTransport(agent.SpanServerAddr(), 5*spanSize+processSize+emitBatchOverhead) + sender, err := NewUDPTransport(agent.SpanServerAddr(), 5*spanSize+processSize+emitBatchOverhead, log.NullLogger) require.NoError(t, err) udpSender := sender.(*udpSender) udpSender.SetReporterStats(&mockRepStats{spansDroppedFromQueue: 5}) @@ -200,7 +201,7 @@ func TestUDPSenderAppend(t *testing.T) { for _, test := range tests { bufferSize := 5*spanSize + test.bufferSizeOffset + processSize + emitBatchOverhead - sender, err := NewUDPTransport(agent.SpanServerAddr(), bufferSize) + sender, err := NewUDPTransport(agent.SpanServerAddr(), bufferSize, log.NullLogger) require.NoError(t, err, test.description) agent.ResetJaegerBatches() @@ -257,7 +258,7 @@ func TestUDPSenderHugeSpan(t *testing.T) { span := newSpan() spanSize := getThriftSpanByteLength(t, span) - sender, err := NewUDPTransport(agent.SpanServerAddr(), spanSize/2+emitBatchOverhead) + sender, err := NewUDPTransport(agent.SpanServerAddr(), spanSize/2+emitBatchOverhead, log.NullLogger) require.NoError(t, err) n, err := sender.Append(span) @@ -272,7 +273,7 @@ func TestUDPSenderHugeSpan(t *testing.T) { } func TestUDPSender_defaultHostPort(t *testing.T) { - tr, err := NewUDPTransport("", 0) + tr, err := NewUDPTransport("", 0, log.NullLogger) require.NoError(t, err) assert.NoError(t, tr.Close()) } diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go new file mode 100644 index 00000000..b97cd91f --- /dev/null +++ b/utils/resolved_udp_conn.go @@ -0,0 +1,169 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// 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. + +package utils + +import ( + "context" + "fmt" + "github.com/uber/jaeger-client-go/log" + "net" + "sync" + "time" +) + +// ResolvedUDPConn is an implementation of udpConn that continually resolves the provided hostname and atomically swaps +// the connection if the dial succeeds +type ResolvedUDPConn struct { + hostPort string + resolveFunc resolveFunc + dialFunc dialFunc + logger log.Logger + + connMtx sync.RWMutex + conn *net.UDPConn + destAddr *net.UDPAddr + closeChan chan struct{} + doneChan chan struct{} +} + +type resolveFunc func(network string, hostPort string) (*net.UDPAddr, error) +type dialFunc func(network string, laddr, raddr *net.UDPAddr) (*net.UDPConn, error) + +// NewResolvedUDPConn returns a new udpConn that resolves hostPort every resolveTimeout, if the resolved address is +// different than the current conn then the new address is dial and the conn is swapped +func NewResolvedUDPConn(hostPort string, resolveTimeout time.Duration, resolveFunc resolveFunc, dialFunc dialFunc, logger log.Logger) (*ResolvedUDPConn, error) { + conn := &ResolvedUDPConn{ + hostPort: hostPort, + resolveFunc: resolveFunc, + dialFunc: dialFunc, + logger: logger, + } + + err := conn.attemptResolveAndDial() + if err != nil { + return nil, err + } + + conn.closeChan = make(chan struct{}) + conn.doneChan = make(chan struct{}) + + go conn.resolveLoop(resolveTimeout) + + return conn, nil +} + +func (c *ResolvedUDPConn) resolveLoop(resolveTimeout time.Duration) { + ticker := time.NewTicker(resolveTimeout) + defer ticker.Stop() + + defer func() { + if c.doneChan != nil { + c.doneChan <- struct{}{} + } + }() + + for { + select { + case <-c.closeChan: + return + case <-ticker.C: + if err := c.attemptResolveAndDial(); err != nil { + c.logger.Error(err.Error()) + } + } + } +} + +func (c *ResolvedUDPConn) attemptResolveAndDial() error { + newAddr, err := c.resolveFunc("udp", c.hostPort) + if err != nil { + return fmt.Errorf("failed to resolve new addr, with err: %w", err) + } + + // dont attempt dial if resolved addr is the same as current conn + if c.destAddr != nil && newAddr.String() == c.destAddr.String() { + return nil + } + + if err := c.attemptDialNewAddr(newAddr); err != nil { + return fmt.Errorf("failed to dial newly resolved addr %s, with err: %v", newAddr.String(), err) + } + + return nil +} + +func (c *ResolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { + connUDP, err := c.dialFunc(newAddr.Network(), nil, newAddr) + if err != nil { + // failed to dial new address + return err + } + + c.connMtx.Lock() + c.destAddr = newAddr + // store prev to close later + prevConn := c.conn + c.conn = connUDP + c.connMtx.Unlock() + + if prevConn != nil { + return prevConn.Close() + } + + return nil +} + +// Write calls net.udpConn.Write, if it fails an attempt is made to connect to a new addr, if that succeeds the write is retried before returning +func (c *ResolvedUDPConn) Write(b []byte) (int, error) { + c.connMtx.RLock() + bytesWritten, err := c.conn.Write(b) + c.connMtx.RUnlock() + + if err == nil { + return bytesWritten, nil + } + + // attempt to resolve and dial new address in case that's the problem, if resolve and dial succeeds, try write again + if reconnErr := c.attemptResolveAndDial(); reconnErr == nil { + c.connMtx.RLock() + defer c.connMtx.RUnlock() + return c.conn.Write(b) + } + + // return original error if reconn fails + return bytesWritten, err +} + +// Close stops the resolveLoop with a deadline of 2 seconds before continuing to close the connection via net.udpConn 's implementation +func (c *ResolvedUDPConn) Close() error { + c.closeChan <- struct{}{} + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*2) + select { + case <-c.doneChan: + case <-ctx.Done(): + c.logger.Error("resolve loop didnt finish within 2 second timeout, closing conn") + } + cancel() + + return c.conn.Close() +} + +// SetWriteBuffer defers to the net.udpConn SetWriteBuffer implementation wrapped with a RLock +func (c *ResolvedUDPConn) SetWriteBuffer(bytes int) error { + c.connMtx.RLock() + defer c.connMtx.RUnlock() + return c.conn.SetWriteBuffer(bytes) +} diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go new file mode 100644 index 00000000..924c1678 --- /dev/null +++ b/utils/resolved_udp_conn_test.go @@ -0,0 +1,168 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// 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. + +package utils + +import ( + "net" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/uber/jaeger-client-go/log" +) + +type mockResolver struct { + mock.Mock +} + +func (m *mockResolver) ResolveUDPAddr(network string, hostPort string) (*net.UDPAddr, error) { + args := m.Called(network, hostPort) + + a0 := args.Get(0) + if a0 == nil { + return (*net.UDPAddr)(nil), args.Error(1) + } + return a0.(*net.UDPAddr), args.Error(1) +} + +type mockDialer struct { + mock.Mock +} + +func (m *mockDialer) DialUDP(network string, laddr, raddr *net.UDPAddr) (*net.UDPConn, error) { + args := m.Called(network, laddr, raddr) + + a0 := args.Get(0) + if a0 == nil { + return (*net.UDPConn)(nil), args.Error(1) + } + + return a0.(*net.UDPConn), args.Error(1) +} + +func newUDPConn() (net.PacketConn, *net.UDPConn, error) { + mockServer, err := net.ListenPacket("udp", "127.0.0.1:0") + if err != nil { + return nil, nil, err + } + + addr, err := net.ResolveUDPAddr("udp", mockServer.LocalAddr().String()) + if err != nil { + mockServer.Close() + return nil, nil, err + } + + conn, err := net.DialUDP("udp", nil, addr) + if err != nil { + mockServer.Close() + return nil, nil, err + } + + return mockServer, conn, nil +} + +func TestNewResolvedUDPConn(t *testing.T) { + hostPort := "blahblah:34322" + + mockServer, clientConn, err := newUDPConn() + if !assert.NoError(t, err) { + t.FailNow() + } + defer mockServer.Close() + + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil). + Once() + + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil). + Once() + + conn, err := NewResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + assert.NoError(t, err) + assert.NotNil(t, conn) + + err = conn.Close() + assert.NoError(t, err) + + // assert the actual connection was closed + assert.Error(t, clientConn.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} + +func TestResolvedUDPConnWrites(t *testing.T) { + hostPort := "blahblah:34322" + + mockServer, clientConn, err := newUDPConn() + if !assert.NoError(t, err) { + t.FailNow() + } + defer mockServer.Close() + + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil). + Once() + + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil). + Once() + + conn, err := NewResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + assert.NoError(t, err) + assert.NotNil(t, conn) + + expectedString := "yo this is a test" + _, err = conn.Write([]byte(expectedString)) + assert.NoError(t, err) + + var buf = make([]byte, len(expectedString)) + err = mockServer.SetReadDeadline(time.Now().Add(time.Second)) + assert.NoError(t, err) + _, _, err = mockServer.ReadFrom(buf) + assert.NoError(t, err) + assert.Equal(t, []byte(expectedString), buf) + + err = conn.Close() + assert.NoError(t, err) + + // assert the actual connection was closed + assert.Error(t, clientConn.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} diff --git a/utils/udp_client.go b/utils/udp_client.go index fadd73e4..6f70bd94 100644 --- a/utils/udp_client.go +++ b/utils/udp_client.go @@ -19,7 +19,9 @@ import ( "fmt" "io" "net" + "time" + "github.com/uber/jaeger-client-go/log" "github.com/uber/jaeger-client-go/thrift" "github.com/uber/jaeger-client-go/thrift-gen/agent" @@ -35,14 +37,20 @@ type AgentClientUDP struct { agent.Agent io.Closer - connUDP *net.UDPConn + connUDP udpConn client *agent.AgentClient maxPacketSize int // max size of datagram in bytes thriftBuffer *thrift.TMemoryBuffer // buffer used to calculate byte size of a span } +type udpConn interface { + Write([]byte) (int, error) + SetWriteBuffer(int) error + Close() error +} + // NewAgentClientUDP creates a client that sends spans to Jaeger Agent over UDP. -func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, error) { +func NewAgentClientUDP(hostPort string, maxPacketSize int, logger log.Logger) (*AgentClientUDP, error) { if maxPacketSize == 0 { maxPacketSize = UDPPacketMaxLength } @@ -51,15 +59,33 @@ func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, err protocolFactory := thrift.NewTCompactProtocolFactory() client := agent.NewAgentClientFactory(thriftBuffer, protocolFactory) - destAddr, err := net.ResolveUDPAddr("udp", hostPort) + var connUDP udpConn + var err error + + host, _, err := net.SplitHostPort(hostPort) if err != nil { return nil, err } - connUDP, err := net.DialUDP(destAddr.Network(), nil, destAddr) - if err != nil { - return nil, err + if ip := net.ParseIP(host); ip == nil { + // host is hostname, setup resolver loop in case host record changes during operation + connUDP, err = NewResolvedUDPConn(hostPort, time.Second*30, net.ResolveUDPAddr, net.DialUDP, logger) + if err != nil { + return nil, err + } + } else { + // have to use resolve even though we know hostPort contains a literal ip, in case address is ipv6 with a zone + destAddr, err := net.ResolveUDPAddr("udp", hostPort) + if err != nil { + return nil, err + } + + connUDP, err = net.DialUDP(destAddr.Network(), nil, destAddr) + if err != nil { + return nil, err + } } + if err := connUDP.SetWriteBuffer(maxPacketSize); err != nil { return nil, err } @@ -68,7 +94,8 @@ func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, err connUDP: connUDP, client: client, maxPacketSize: maxPacketSize, - thriftBuffer: thriftBuffer} + thriftBuffer: thriftBuffer, + } return clientUDP, nil } From 3690bcb9bea3f0c9405339184ed109ca062c2a81 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Fri, 26 Jun 2020 03:19:57 -0400 Subject: [PATCH 02/25] Be sure to set buffer bytes width on new connections. Signed-off-by: Trevor Foster --- utils/resolved_udp_conn.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go index b97cd91f..e9a3c333 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/resolved_udp_conn.go @@ -30,6 +30,7 @@ type ResolvedUDPConn struct { resolveFunc resolveFunc dialFunc dialFunc logger log.Logger + bufferBytes int connMtx sync.RWMutex conn *net.UDPConn @@ -111,6 +112,13 @@ func (c *ResolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { return err } + if c.bufferBytes != 0 { + err = connUDP.SetWriteBuffer(c.bufferBytes) + if err != nil { + return err + } + } + c.connMtx.Lock() c.destAddr = newAddr // store prev to close later @@ -164,6 +172,12 @@ func (c *ResolvedUDPConn) Close() error { // SetWriteBuffer defers to the net.udpConn SetWriteBuffer implementation wrapped with a RLock func (c *ResolvedUDPConn) SetWriteBuffer(bytes int) error { c.connMtx.RLock() - defer c.connMtx.RUnlock() - return c.conn.SetWriteBuffer(bytes) + err := c.conn.SetWriteBuffer(bytes) + c.connMtx.RUnlock() + + if err != nil { + c.bufferBytes = bytes + } + + return err } From e7e7b323c930256f021654da01b31ebba165de26 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Fri, 26 Jun 2020 03:38:32 -0400 Subject: [PATCH 03/25] Lock when checking if resolved addr is new. Signed-off-by: Trevor Foster --- utils/resolved_udp_conn.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go index e9a3c333..b526211f 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/resolved_udp_conn.go @@ -93,10 +93,13 @@ func (c *ResolvedUDPConn) attemptResolveAndDial() error { return fmt.Errorf("failed to resolve new addr, with err: %w", err) } + c.connMtx.RLock() // dont attempt dial if resolved addr is the same as current conn if c.destAddr != nil && newAddr.String() == c.destAddr.String() { + c.connMtx.RUnlock() return nil } + c.connMtx.RUnlock() if err := c.attemptDialNewAddr(newAddr); err != nil { return fmt.Errorf("failed to dial newly resolved addr %s, with err: %v", newAddr.String(), err) From 7994cdf4ba52769077941373cb577b9e8509bfa2 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Mon, 29 Jun 2020 02:48:27 -0400 Subject: [PATCH 04/25] Fixes from review comments. Dont return error if UDPConn fails on startup Signed-off-by: Trevor Foster --- config/config.go | 5 +- config/config_test.go | 2 +- reporter_test.go | 2 +- testutils/mock_agent.go | 3 +- transport_udp.go | 31 +++++++- transport_udp_test.go | 9 +-- utils/resolved_udp_conn.go | 99 +++++++++++++----------- utils/resolved_udp_conn_test.go | 129 +++++++++++++++++++++++++++++++- utils/udp_client.go | 32 ++++++-- 9 files changed, 245 insertions(+), 67 deletions(-) diff --git a/config/config.go b/config/config.go index b2c49dbe..866af6bf 100644 --- a/config/config.go +++ b/config/config.go @@ -410,6 +410,9 @@ func (rc *ReporterConfig) newTransport(logger jaeger.Logger) (jaeger.Transport, } return transport.NewHTTPTransport(rc.CollectorEndpoint, httpOptions...), nil default: - return jaeger.NewUDPTransport(rc.LocalAgentHostPort, 0, logger) + return jaeger.NewUDPTransportWithParams(jaeger.UDPTransportParams{ + HostPort: rc.LocalAgentHostPort, + Logger: logger, + }) } } diff --git a/config/config_test.go b/config/config_test.go index ad67df5f..f7ae84a8 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -560,7 +560,7 @@ func TestInvalidSamplerType(t *testing.T) { func TestUDPTransportType(t *testing.T) { rc := &ReporterConfig{LocalAgentHostPort: "localhost:1234"} - expect, _ := jaeger.NewUDPTransport(rc.LocalAgentHostPort, 0, log.NullLogger) + expect, _ := jaeger.NewUDPTransport(rc.LocalAgentHostPort, 0) sender, err := rc.newTransport(log.NullLogger) require.NoError(t, err) require.IsType(t, expect, sender) diff --git a/reporter_test.go b/reporter_test.go index 64166244..492e9cea 100644 --- a/reporter_test.go +++ b/reporter_test.go @@ -213,7 +213,7 @@ func TestUDPReporter(t *testing.T) { testRemoteReporterWithSender(t, func(m *Metrics) (Transport, error) { - return NewUDPTransport(agent.SpanServerAddr(), 0, log.NullLogger) + return NewUDPTransport(agent.SpanServerAddr(), 0) }, func() []*j.Batch { return agent.GetJaegerBatches() diff --git a/testutils/mock_agent.go b/testutils/mock_agent.go index 1ee32320..dd5e0c0d 100644 --- a/testutils/mock_agent.go +++ b/testutils/mock_agent.go @@ -23,7 +23,6 @@ import ( "sync" "sync/atomic" - "github.com/uber/jaeger-client-go/log" "github.com/uber/jaeger-client-go/thrift" "github.com/uber/jaeger-client-go/thrift-gen/agent" @@ -83,7 +82,7 @@ func (s *MockAgent) SpanServerAddr() string { // SpanServerClient returns a UDP client that can be used to send spans to the MockAgent func (s *MockAgent) SpanServerClient() (agent.Agent, error) { - return utils.NewAgentClientUDP(s.SpanServerAddr(), 0, log.NullLogger) + return utils.NewAgentClientUDP(s.SpanServerAddr(), 0) } // SamplingServerAddr returns the host:port of HTTP server exposing sampling strategy endpoint diff --git a/transport_udp.go b/transport_udp.go index 309dbdf0..05a2acde 100644 --- a/transport_udp.go +++ b/transport_udp.go @@ -58,12 +58,19 @@ type udpSender struct { failedToEmitSpans int64 } -// NewUDPTransport creates a reporter that submits spans to jaeger-agent. -// TODO: (breaking change) move to transport/ package. -func NewUDPTransport(hostPort string, maxPacketSize int, logger log.Logger) (Transport, error) { +// UDPTransportParams allows specifying options for initializing a UDPTransport. An instance of this struct should +// be passed to NewUDPTransportWithParams. +type UDPTransportParams struct { + HostPort string + MaxPacketSize int + Logger log.Logger +} + +func initializeUDPTransport(hostPort string, maxPacketSize int, logger log.Logger) (*udpSender, error) { if len(hostPort) == 0 { hostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) } + if maxPacketSize == 0 { maxPacketSize = utils.UDPPacketMaxLength } @@ -74,7 +81,11 @@ func NewUDPTransport(hostPort string, maxPacketSize int, logger log.Logger) (Tra thriftBuffer := thrift.NewTMemoryBufferLen(maxPacketSize) thriftProtocol := protocolFactory.GetProtocol(thriftBuffer) - client, err := utils.NewAgentClientUDP(hostPort, maxPacketSize, logger) + client, err := utils.NewAgentClientUDPWithParams(utils.AgentClientUDPParams{ + HostPort: hostPort, + MaxPacketSize: maxPacketSize, + Logger: logger, + }) if err != nil { return nil, err } @@ -87,6 +98,18 @@ func NewUDPTransport(hostPort string, maxPacketSize int, logger log.Logger) (Tra }, nil } +// NewUDPTransportWithParams creates a reporter that submits spans to jaeger-agent. +// TODO: (breaking change) move to transport/ package. +func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { + return initializeUDPTransport(params.HostPort, params.MaxPacketSize, params.Logger) +} + +// NewUDPTransport creates a reporter that submits spans to jaeger-agent. +// TODO: (breaking change) move to transport/ package. +func NewUDPTransport(hostPort string, maxPacketSize int) (Transport, error) { + return initializeUDPTransport(hostPort, maxPacketSize, nil) +} + // SetReporterStats implements reporterstats.Receiver. func (s *udpSender) SetReporterStats(rs reporterstats.ReporterStats) { s.reporterStats = rs diff --git a/transport_udp_test.go b/transport_udp_test.go index e5f14572..a0c55894 100644 --- a/transport_udp_test.go +++ b/transport_udp_test.go @@ -22,7 +22,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/uber/jaeger-client-go/log" "github.com/uber/jaeger-client-go/internal/reporterstats" "github.com/uber/jaeger-client-go/testutils" @@ -126,7 +125,7 @@ func TestUDPSenderFlush(t *testing.T) { spanSize := getThriftSpanByteLength(t, span) processSize := getThriftProcessByteLengthFromTracer(t, jaegerTracer) - sender, err := NewUDPTransport(agent.SpanServerAddr(), 5*spanSize+processSize+emitBatchOverhead, log.NullLogger) + sender, err := NewUDPTransport(agent.SpanServerAddr(), 5*spanSize+processSize+emitBatchOverhead) require.NoError(t, err) udpSender := sender.(*udpSender) udpSender.SetReporterStats(&mockRepStats{spansDroppedFromQueue: 5}) @@ -201,7 +200,7 @@ func TestUDPSenderAppend(t *testing.T) { for _, test := range tests { bufferSize := 5*spanSize + test.bufferSizeOffset + processSize + emitBatchOverhead - sender, err := NewUDPTransport(agent.SpanServerAddr(), bufferSize, log.NullLogger) + sender, err := NewUDPTransport(agent.SpanServerAddr(), bufferSize) require.NoError(t, err, test.description) agent.ResetJaegerBatches() @@ -258,7 +257,7 @@ func TestUDPSenderHugeSpan(t *testing.T) { span := newSpan() spanSize := getThriftSpanByteLength(t, span) - sender, err := NewUDPTransport(agent.SpanServerAddr(), spanSize/2+emitBatchOverhead, log.NullLogger) + sender, err := NewUDPTransport(agent.SpanServerAddr(), spanSize/2+emitBatchOverhead) require.NoError(t, err) n, err := sender.Append(span) @@ -273,7 +272,7 @@ func TestUDPSenderHugeSpan(t *testing.T) { } func TestUDPSender_defaultHostPort(t *testing.T) { - tr, err := NewUDPTransport("", 0, log.NullLogger) + tr, err := NewUDPTransport("", 0) require.NoError(t, err) assert.NoError(t, tr.Close()) } diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go index b526211f..84e74230 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/resolved_udp_conn.go @@ -15,17 +15,17 @@ package utils import ( - "context" "fmt" - "github.com/uber/jaeger-client-go/log" "net" "sync" "time" + + "github.com/uber/jaeger-client-go/log" ) -// ResolvedUDPConn is an implementation of udpConn that continually resolves the provided hostname and atomically swaps +// resolvedUDPConn is an implementation of udpConn that continually resolves the provided hostname and atomically swaps // the connection if the dial succeeds -type ResolvedUDPConn struct { +type resolvedUDPConn struct { hostPort string resolveFunc resolveFunc dialFunc dialFunc @@ -36,16 +36,15 @@ type ResolvedUDPConn struct { conn *net.UDPConn destAddr *net.UDPAddr closeChan chan struct{} - doneChan chan struct{} } type resolveFunc func(network string, hostPort string) (*net.UDPAddr, error) type dialFunc func(network string, laddr, raddr *net.UDPAddr) (*net.UDPConn, error) -// NewResolvedUDPConn returns a new udpConn that resolves hostPort every resolveTimeout, if the resolved address is -// different than the current conn then the new address is dial and the conn is swapped -func NewResolvedUDPConn(hostPort string, resolveTimeout time.Duration, resolveFunc resolveFunc, dialFunc dialFunc, logger log.Logger) (*ResolvedUDPConn, error) { - conn := &ResolvedUDPConn{ +// newResolvedUDPConn returns a new udpConn that resolves hostPort every resolveTimeout, if the resolved address is +// different than the current conn then the new address is dialed and the conn is swapped. +func newResolvedUDPConn(hostPort string, resolveTimeout time.Duration, resolveFunc resolveFunc, dialFunc dialFunc, logger log.Logger) (*resolvedUDPConn, error) { + conn := &resolvedUDPConn{ hostPort: hostPort, resolveFunc: resolveFunc, dialFunc: dialFunc, @@ -54,27 +53,20 @@ func NewResolvedUDPConn(hostPort string, resolveTimeout time.Duration, resolveFu err := conn.attemptResolveAndDial() if err != nil { - return nil, err + logger.Error(fmt.Sprintf("failed resolving destination address on connection startup, with err: %q. retrying in %s", err.Error(), resolveTimeout.String())) } conn.closeChan = make(chan struct{}) - conn.doneChan = make(chan struct{}) go conn.resolveLoop(resolveTimeout) return conn, nil } -func (c *ResolvedUDPConn) resolveLoop(resolveTimeout time.Duration) { +func (c *resolvedUDPConn) resolveLoop(resolveTimeout time.Duration) { ticker := time.NewTicker(resolveTimeout) defer ticker.Stop() - defer func() { - if c.doneChan != nil { - c.doneChan <- struct{}{} - } - }() - for { select { case <-c.closeChan: @@ -87,37 +79,41 @@ func (c *ResolvedUDPConn) resolveLoop(resolveTimeout time.Duration) { } } -func (c *ResolvedUDPConn) attemptResolveAndDial() error { +func (c *resolvedUDPConn) attemptResolveAndDial() error { newAddr, err := c.resolveFunc("udp", c.hostPort) if err != nil { - return fmt.Errorf("failed to resolve new addr, with err: %w", err) + return fmt.Errorf("failed to resolve new addr for host %q, with err: %w", c.hostPort, err) } c.connMtx.RLock() - // dont attempt dial if resolved addr is the same as current conn - if c.destAddr != nil && newAddr.String() == c.destAddr.String() { - c.connMtx.RUnlock() + curAddr := c.destAddr + c.connMtx.RUnlock() + + // dont attempt dial if an addr was successfully dialed previously and, resolved addr is the same as current conn + if curAddr != nil && newAddr.String() == curAddr.String() { return nil } - c.connMtx.RUnlock() if err := c.attemptDialNewAddr(newAddr); err != nil { - return fmt.Errorf("failed to dial newly resolved addr %s, with err: %v", newAddr.String(), err) + return fmt.Errorf("failed to dial newly resolved addr %q, with err: %w", newAddr.String(), err) } return nil } -func (c *ResolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { +func (c *resolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { connUDP, err := c.dialFunc(newAddr.Network(), nil, newAddr) if err != nil { // failed to dial new address return err } - if c.bufferBytes != 0 { - err = connUDP.SetWriteBuffer(c.bufferBytes) - if err != nil { + c.connMtx.RLock() + bufferBytes := c.bufferBytes + c.connMtx.RUnlock() + + if bufferBytes != 0 { + if err = connUDP.SetWriteBuffer(bufferBytes); err != nil { return err } } @@ -137,9 +133,17 @@ func (c *ResolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { } // Write calls net.udpConn.Write, if it fails an attempt is made to connect to a new addr, if that succeeds the write is retried before returning -func (c *ResolvedUDPConn) Write(b []byte) (int, error) { +func (c *resolvedUDPConn) Write(b []byte) (int, error) { + var bytesWritten int + var err error + c.connMtx.RLock() - bytesWritten, err := c.conn.Write(b) + if c.conn == nil { + // if connection is not initialized indicate this with err in order to hook into retry logic + err = fmt.Errorf("UDP connection not yet initialized, a valid address has not been resolved") + } else { + bytesWritten, err = c.conn.Write(b) + } c.connMtx.RUnlock() if err == nil { @@ -157,29 +161,34 @@ func (c *ResolvedUDPConn) Write(b []byte) (int, error) { return bytesWritten, err } -// Close stops the resolveLoop with a deadline of 2 seconds before continuing to close the connection via net.udpConn 's implementation -func (c *ResolvedUDPConn) Close() error { - c.closeChan <- struct{}{} +// Close stops the resolveLoop, then closes the connection via net.udpConn 's implementation +func (c *resolvedUDPConn) Close() error { + close(c.closeChan) + c.connMtx.RLock() + defer c.connMtx.RUnlock() - ctx, cancel := context.WithTimeout(context.Background(), time.Second*2) - select { - case <-c.doneChan: - case <-ctx.Done(): - c.logger.Error("resolve loop didnt finish within 2 second timeout, closing conn") + if c.conn != nil { + return c.conn.Close() } - cancel() - return c.conn.Close() + return nil } -// SetWriteBuffer defers to the net.udpConn SetWriteBuffer implementation wrapped with a RLock -func (c *ResolvedUDPConn) SetWriteBuffer(bytes int) error { +// SetWriteBuffer defers to the net.udpConn SetWriteBuffer implementation wrapped with a RLock. if no conn is currently held +// and SetWriteBuffer is called store bufferBytes to be set for new conns +func (c *resolvedUDPConn) SetWriteBuffer(bytes int) error { + var err error + c.connMtx.RLock() - err := c.conn.SetWriteBuffer(bytes) + if c.conn != nil { + err = c.conn.SetWriteBuffer(bytes) + } c.connMtx.RUnlock() - if err != nil { + if err == nil { + c.connMtx.Lock() c.bufferBytes = bytes + c.connMtx.Unlock() } return err diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index 924c1678..dfd188d8 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -15,7 +15,9 @@ package utils import ( + "fmt" "net" + "syscall" "testing" "time" @@ -101,7 +103,7 @@ func TestNewResolvedUDPConn(t *testing.T) { Return(clientConn, nil). Once() - conn, err := NewResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) assert.NotNil(t, conn) @@ -142,7 +144,7 @@ func TestResolvedUDPConnWrites(t *testing.T) { Return(clientConn, nil). Once() - conn, err := NewResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) assert.NotNil(t, conn) @@ -166,3 +168,126 @@ func TestResolvedUDPConnWrites(t *testing.T) { resolver.AssertExpectations(t) dialer.AssertExpectations(t) } + +func TestResolvedUDPConnEventuallyDials(t *testing.T) { + hostPort := "blahblah:34322" + + mockServer, clientConn, err := newUDPConn() + if !assert.NoError(t, err) { + t.FailNow() + } + defer mockServer.Close() + + resolver := mockResolver{} + resolveCall := resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(nil, fmt.Errorf("failed to resolve")) + + timer := time.AfterFunc(time.Millisecond*100, func() { + resolveCall.Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil) + }) + defer timer.Stop() + + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil). + Once() + + conn, err := newResolvedUDPConn(hostPort, time.Millisecond*100, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + assert.NoError(t, err) + assert.NotNil(t, conn) + + err = conn.SetWriteBuffer(65000) + assert.NoError(t, err) + + <-time.After(time.Millisecond * 200) + + fd, _ := clientConn.File() + bufferBytes, _ := syscall.GetsockoptInt(int(fd.Fd()), syscall.SOL_SOCKET, syscall.SO_SNDBUF) + assert.Equal(t, 65000, bufferBytes) + + expectedString := "yo this is a test" + _, err = conn.Write([]byte(expectedString)) + assert.NoError(t, err) + + var buf = make([]byte, len(expectedString)) + err = mockServer.SetReadDeadline(time.Now().Add(time.Second)) + assert.NoError(t, err) + _, _, err = mockServer.ReadFrom(buf) + assert.NoError(t, err) + assert.Equal(t, []byte(expectedString), buf) + + err = conn.Close() + assert.NoError(t, err) + + // assert the actual connection was closed + assert.Error(t, clientConn.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} + +func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { + hostPort := "blahblah:34322" + + mockServer, clientConn, err := newUDPConn() + if !assert.NoError(t, err) { + t.FailNow() + } + defer mockServer.Close() + + resolver := mockResolver{} + resolveCall := resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil) + + // Simulate resolve fail after 100 seconds to ensure connection isn't swapped + timer := time.AfterFunc(time.Millisecond*100, func() { + resolveCall.Return(nil, fmt.Errorf("resolve failed")) + }) + defer timer.Stop() + + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil). + Once() + + conn, err := newResolvedUDPConn(hostPort, time.Millisecond*100, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + assert.NoError(t, err) + assert.NotNil(t, conn) + + <-time.After(time.Millisecond * 200) + expectedString := "yo this is a test" + _, err = conn.Write([]byte(expectedString)) + assert.NoError(t, err) + + var buf = make([]byte, len(expectedString)) + err = mockServer.SetReadDeadline(time.Now().Add(time.Second)) + assert.NoError(t, err) + _, _, err = mockServer.ReadFrom(buf) + assert.NoError(t, err) + assert.Equal(t, []byte(expectedString), buf) + + err = conn.Close() + assert.NoError(t, err) + + // assert the actual connection was closed + assert.Error(t, clientConn.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} diff --git a/utils/udp_client.go b/utils/udp_client.go index 6f70bd94..f9d6d2ee 100644 --- a/utils/udp_client.go +++ b/utils/udp_client.go @@ -49,12 +49,23 @@ type udpConn interface { Close() error } -// NewAgentClientUDP creates a client that sends spans to Jaeger Agent over UDP. -func NewAgentClientUDP(hostPort string, maxPacketSize int, logger log.Logger) (*AgentClientUDP, error) { +// AgentClientUDPParams allows specifying options for initializing an AgentClientUDP. An instance of this struct should +// be passed to NewAgentClientUDPWithParams. +type AgentClientUDPParams struct { + HostPort string + MaxPacketSize int + Logger log.Logger +} + +func initializeAgentClientUDP(hostPort string, maxPacketSize int, logger log.Logger) (*AgentClientUDP, error) { if maxPacketSize == 0 { maxPacketSize = UDPPacketMaxLength } + if logger == nil { + logger = log.StdLogger + } + thriftBuffer := thrift.NewTMemoryBufferLen(maxPacketSize) protocolFactory := thrift.NewTCompactProtocolFactory() client := agent.NewAgentClientFactory(thriftBuffer, protocolFactory) @@ -69,7 +80,7 @@ func NewAgentClientUDP(hostPort string, maxPacketSize int, logger log.Logger) (* if ip := net.ParseIP(host); ip == nil { // host is hostname, setup resolver loop in case host record changes during operation - connUDP, err = NewResolvedUDPConn(hostPort, time.Second*30, net.ResolveUDPAddr, net.DialUDP, logger) + connUDP, err = newResolvedUDPConn(hostPort, time.Second*30, net.ResolveUDPAddr, net.DialUDP, logger) if err != nil { return nil, err } @@ -90,13 +101,22 @@ func NewAgentClientUDP(hostPort string, maxPacketSize int, logger log.Logger) (* return nil, err } - clientUDP := &AgentClientUDP{ + return &AgentClientUDP{ connUDP: connUDP, client: client, maxPacketSize: maxPacketSize, thriftBuffer: thriftBuffer, - } - return clientUDP, nil + }, nil +} + +// NewAgentClientUDPWithParams creates a client that sends spans to Jaeger Agent over UDP. +func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, error) { + return initializeAgentClientUDP(params.HostPort, params.MaxPacketSize, params.Logger) +} + +// NewAgentClientUDP creates a client that sends spans to Jaeger Agent over UDP. +func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, error) { + return initializeAgentClientUDP(hostPort, maxPacketSize, nil) } // EmitZipkinBatch implements EmitZipkinBatch() of Agent interface From 66b7ec4ecc8dc7287dcfe1145dc5b481819e2641 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Mon, 29 Jun 2020 03:10:47 -0400 Subject: [PATCH 05/25] Fix failing test. Apparently the linux kernel returns the sockopt val doubled. Signed-off-by: Trevor Foster --- utils/resolved_udp_conn_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index dfd188d8..b9b4e319 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -17,6 +17,7 @@ package utils import ( "fmt" "net" + "runtime" "syscall" "testing" "time" @@ -211,7 +212,11 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { fd, _ := clientConn.File() bufferBytes, _ := syscall.GetsockoptInt(int(fd.Fd()), syscall.SOL_SOCKET, syscall.SO_SNDBUF) - assert.Equal(t, 65000, bufferBytes) + if runtime.GOOS == "darwin" { + assert.Equal(t, 65000, bufferBytes) + } else { + assert.Equal(t, 65000*2, bufferBytes) + } expectedString := "yo this is a test" _, err = conn.Write([]byte(expectedString)) From fc0d3ebdc85dc8e80d46729c40836b928f1cb4cc Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Mon, 29 Jun 2020 16:30:24 -0400 Subject: [PATCH 06/25] Use atomic ops to manage bufferBytes instead of locking mutex Signed-off-by: Trevor Foster --- utils/resolved_udp_conn.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go index 84e74230..cfe17e9a 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/resolved_udp_conn.go @@ -18,6 +18,7 @@ import ( "fmt" "net" "sync" + "sync/atomic" "time" "github.com/uber/jaeger-client-go/log" @@ -30,7 +31,7 @@ type resolvedUDPConn struct { resolveFunc resolveFunc dialFunc dialFunc logger log.Logger - bufferBytes int + bufferBytes int64 connMtx sync.RWMutex conn *net.UDPConn @@ -108,9 +109,7 @@ func (c *resolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { return err } - c.connMtx.RLock() - bufferBytes := c.bufferBytes - c.connMtx.RUnlock() + bufferBytes := int(atomic.LoadInt64(&c.bufferBytes)) if bufferBytes != 0 { if err = connUDP.SetWriteBuffer(bufferBytes); err != nil { @@ -186,9 +185,7 @@ func (c *resolvedUDPConn) SetWriteBuffer(bytes int) error { c.connMtx.RUnlock() if err == nil { - c.connMtx.Lock() - c.bufferBytes = bytes - c.connMtx.Unlock() + atomic.StoreInt64(&c.bufferBytes, int64(bytes)) } return err From c47cc4b2f27629341ab14989ef30371c185dace6 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Mon, 29 Jun 2020 17:35:03 -0400 Subject: [PATCH 07/25] Fix buffer bytes assert because sock opt value is not guaranteed to be exactly twice set val Signed-off-by: Trevor Foster --- utils/resolved_udp_conn_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index b9b4e319..68d75051 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -215,7 +215,7 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { if runtime.GOOS == "darwin" { assert.Equal(t, 65000, bufferBytes) } else { - assert.Equal(t, 65000*2, bufferBytes) + assert.GreaterOrEqual(t, 65000*2, bufferBytes) } expectedString := "yo this is a test" From aaf0c9b420e28648794f01cdc4c3ace3f9c74220 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Tue, 30 Jun 2020 15:35:15 -0400 Subject: [PATCH 08/25] Remove intermediate init helpers, initialize close chann in struct initialization Signed-off-by: Trevor Foster --- transport_udp.go | 33 ++++++++++++++++----------------- utils/resolved_udp_conn.go | 13 ++++++------- utils/udp_client.go | 33 ++++++++++++++++----------------- 3 files changed, 38 insertions(+), 41 deletions(-) diff --git a/transport_udp.go b/transport_udp.go index 05a2acde..722d3cca 100644 --- a/transport_udp.go +++ b/transport_udp.go @@ -66,25 +66,27 @@ type UDPTransportParams struct { Logger log.Logger } -func initializeUDPTransport(hostPort string, maxPacketSize int, logger log.Logger) (*udpSender, error) { - if len(hostPort) == 0 { - hostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) +// NewUDPTransportWithParams creates a reporter that submits spans to jaeger-agent. +// TODO: (breaking change) move to transport/ package. +func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { + if len(params.HostPort) == 0 { + params.HostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) } - if maxPacketSize == 0 { - maxPacketSize = utils.UDPPacketMaxLength + if params.MaxPacketSize == 0 { + params.MaxPacketSize = utils.UDPPacketMaxLength } protocolFactory := thrift.NewTCompactProtocolFactory() // Each span is first written to thriftBuffer to determine its size in bytes. - thriftBuffer := thrift.NewTMemoryBufferLen(maxPacketSize) + thriftBuffer := thrift.NewTMemoryBufferLen(params.MaxPacketSize) thriftProtocol := protocolFactory.GetProtocol(thriftBuffer) client, err := utils.NewAgentClientUDPWithParams(utils.AgentClientUDPParams{ - HostPort: hostPort, - MaxPacketSize: maxPacketSize, - Logger: logger, + HostPort: params.HostPort, + MaxPacketSize: params.MaxPacketSize, + Logger: params.Logger, }) if err != nil { return nil, err @@ -92,22 +94,19 @@ func initializeUDPTransport(hostPort string, maxPacketSize int, logger log.Logge return &udpSender{ client: client, - maxSpanBytes: maxPacketSize - emitBatchOverhead, + maxSpanBytes: params.MaxPacketSize - emitBatchOverhead, thriftBuffer: thriftBuffer, thriftProtocol: thriftProtocol, }, nil } -// NewUDPTransportWithParams creates a reporter that submits spans to jaeger-agent. -// TODO: (breaking change) move to transport/ package. -func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { - return initializeUDPTransport(params.HostPort, params.MaxPacketSize, params.Logger) -} - // NewUDPTransport creates a reporter that submits spans to jaeger-agent. // TODO: (breaking change) move to transport/ package. func NewUDPTransport(hostPort string, maxPacketSize int) (Transport, error) { - return initializeUDPTransport(hostPort, maxPacketSize, nil) + return NewUDPTransportWithParams(UDPTransportParams{ + HostPort: hostPort, + MaxPacketSize: maxPacketSize, + }) } // SetReporterStats implements reporterstats.Receiver. diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go index cfe17e9a..0b2865f6 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/resolved_udp_conn.go @@ -50,15 +50,13 @@ func newResolvedUDPConn(hostPort string, resolveTimeout time.Duration, resolveFu resolveFunc: resolveFunc, dialFunc: dialFunc, logger: logger, + closeChan: make(chan struct{}), } - err := conn.attemptResolveAndDial() - if err != nil { - logger.Error(fmt.Sprintf("failed resolving destination address on connection startup, with err: %q. retrying in %s", err.Error(), resolveTimeout.String())) + if err := conn.attemptResolveAndDial(); err != nil { + logger.Error(fmt.Sprintf("failed resolving destination address on connection startup, with err: %q. retrying in %s", err.Error(), resolveTimeout)) } - conn.closeChan = make(chan struct{}) - go conn.resolveLoop(resolveTimeout) return conn, nil @@ -96,7 +94,7 @@ func (c *resolvedUDPConn) attemptResolveAndDial() error { } if err := c.attemptDialNewAddr(newAddr); err != nil { - return fmt.Errorf("failed to dial newly resolved addr %q, with err: %w", newAddr.String(), err) + return fmt.Errorf("failed to dial newly resolved addr '%s', with err: %w", newAddr, err) } return nil @@ -105,7 +103,6 @@ func (c *resolvedUDPConn) attemptResolveAndDial() error { func (c *resolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { connUDP, err := c.dialFunc(newAddr.Network(), nil, newAddr) if err != nil { - // failed to dial new address return err } @@ -163,6 +160,8 @@ func (c *resolvedUDPConn) Write(b []byte) (int, error) { // Close stops the resolveLoop, then closes the connection via net.udpConn 's implementation func (c *resolvedUDPConn) Close() error { close(c.closeChan) + + // aqcuire rlock before closing conn to ensure calls to Write drain c.connMtx.RLock() defer c.connMtx.RUnlock() diff --git a/utils/udp_client.go b/utils/udp_client.go index f9d6d2ee..40fe661e 100644 --- a/utils/udp_client.go +++ b/utils/udp_client.go @@ -57,36 +57,37 @@ type AgentClientUDPParams struct { Logger log.Logger } -func initializeAgentClientUDP(hostPort string, maxPacketSize int, logger log.Logger) (*AgentClientUDP, error) { - if maxPacketSize == 0 { - maxPacketSize = UDPPacketMaxLength +// NewAgentClientUDPWithParams creates a client that sends spans to Jaeger Agent over UDP. +func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, error) { + if params.MaxPacketSize == 0 { + params.MaxPacketSize = UDPPacketMaxLength } - if logger == nil { - logger = log.StdLogger + if params.Logger == nil { + params.Logger = log.StdLogger } - thriftBuffer := thrift.NewTMemoryBufferLen(maxPacketSize) + thriftBuffer := thrift.NewTMemoryBufferLen(params.MaxPacketSize) protocolFactory := thrift.NewTCompactProtocolFactory() client := agent.NewAgentClientFactory(thriftBuffer, protocolFactory) var connUDP udpConn var err error - host, _, err := net.SplitHostPort(hostPort) + host, _, err := net.SplitHostPort(params.HostPort) if err != nil { return nil, err } if ip := net.ParseIP(host); ip == nil { // host is hostname, setup resolver loop in case host record changes during operation - connUDP, err = newResolvedUDPConn(hostPort, time.Second*30, net.ResolveUDPAddr, net.DialUDP, logger) + connUDP, err = newResolvedUDPConn(params.HostPort, time.Second*30, net.ResolveUDPAddr, net.DialUDP, params.Logger) if err != nil { return nil, err } } else { // have to use resolve even though we know hostPort contains a literal ip, in case address is ipv6 with a zone - destAddr, err := net.ResolveUDPAddr("udp", hostPort) + destAddr, err := net.ResolveUDPAddr("udp", params.HostPort) if err != nil { return nil, err } @@ -97,26 +98,24 @@ func initializeAgentClientUDP(hostPort string, maxPacketSize int, logger log.Log } } - if err := connUDP.SetWriteBuffer(maxPacketSize); err != nil { + if err := connUDP.SetWriteBuffer(params.MaxPacketSize); err != nil { return nil, err } return &AgentClientUDP{ connUDP: connUDP, client: client, - maxPacketSize: maxPacketSize, + maxPacketSize: params.MaxPacketSize, thriftBuffer: thriftBuffer, }, nil } -// NewAgentClientUDPWithParams creates a client that sends spans to Jaeger Agent over UDP. -func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, error) { - return initializeAgentClientUDP(params.HostPort, params.MaxPacketSize, params.Logger) -} - // NewAgentClientUDP creates a client that sends spans to Jaeger Agent over UDP. func NewAgentClientUDP(hostPort string, maxPacketSize int) (*AgentClientUDP, error) { - return initializeAgentClientUDP(hostPort, maxPacketSize, nil) + return NewAgentClientUDPWithParams(AgentClientUDPParams{ + HostPort: hostPort, + MaxPacketSize: maxPacketSize, + }) } // EmitZipkinBatch implements EmitZipkinBatch() of Agent interface From 0264dd88e10823a4cb8316ff14862f776b4079ac Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Thu, 2 Jul 2020 23:01:35 -0400 Subject: [PATCH 09/25] Fixes based on comments, more tests for udp_client.go, and test for write retry Signed-off-by: Trevor Foster --- transport_udp.go | 4 + utils/resolved_udp_conn.go | 6 +- utils/resolved_udp_conn_test.go | 198 ++++++++++++++++++++------------ utils/udp_client.go | 16 ++- utils/udp_client_test.go | 168 +++++++++++++++++++++++++++ 5 files changed, 314 insertions(+), 78 deletions(-) create mode 100644 utils/udp_client_test.go diff --git a/transport_udp.go b/transport_udp.go index 722d3cca..fd57e308 100644 --- a/transport_udp.go +++ b/transport_udp.go @@ -77,6 +77,10 @@ func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { params.MaxPacketSize = utils.UDPPacketMaxLength } + if params.Logger == nil { + params.Logger = log.StdLogger + } + protocolFactory := thrift.NewTCompactProtocolFactory() // Each span is first written to thriftBuffer to determine its size in bytes. diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go index 0b2865f6..759ae05f 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/resolved_udp_conn.go @@ -106,9 +106,7 @@ func (c *resolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { return err } - bufferBytes := int(atomic.LoadInt64(&c.bufferBytes)) - - if bufferBytes != 0 { + if bufferBytes := int(atomic.LoadInt64(&c.bufferBytes)); bufferBytes != 0 { if err = connUDP.SetWriteBuffer(bufferBytes); err != nil { return err } @@ -136,7 +134,7 @@ func (c *resolvedUDPConn) Write(b []byte) (int, error) { c.connMtx.RLock() if c.conn == nil { // if connection is not initialized indicate this with err in order to hook into retry logic - err = fmt.Errorf("UDP connection not yet initialized, a valid address has not been resolved") + err = fmt.Errorf("UDP connection not yet initialized, an address has not been resolved") } else { bytesWritten, err = c.conn.Write(b) } diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index 68d75051..0037b1ce 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -15,6 +15,7 @@ package utils import ( + "context" "fmt" "net" "runtime" @@ -24,10 +25,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "github.com/uber/jaeger-client-go/log" ) type mockResolver struct { + MockHost string mock.Mock } @@ -77,13 +80,42 @@ func newUDPConn() (net.PacketConn, *net.UDPConn, error) { return mockServer, conn, nil } +func assertSockBufferSize(t *testing.T, expectedBytes int, conn *net.UDPConn) bool { + fd, _ := conn.File() + bufferBytes, _ := syscall.GetsockoptInt(int(fd.Fd()), syscall.SOL_SOCKET, syscall.SO_SNDBUF) + + // The linux kernel doubles SO_SNDBUF value (to allow space for bookkeeping overhead) when it is set using setsockopt(2), and this doubled value is returned by getsockopt(2) + // https://linux.die.net/man/7/socket + if runtime.GOOS == "linux" { + return assert.GreaterOrEqual(t, expectedBytes*2, bufferBytes) + } else { + return assert.Equal(t, expectedBytes, bufferBytes) + } +} + +func assertConnWritable(t *testing.T, conn udpConn, serverConn net.PacketConn) (allSucceed bool) { + expectedString := "yo this is a test" + _, err := conn.Write([]byte(expectedString)) + allSucceed = assert.NoError(t, err) + + var buf = make([]byte, len(expectedString)) + err = serverConn.SetReadDeadline(time.Now().Add(time.Second)) + assertion := assert.NoError(t, err) + allSucceed = allSucceed && assertion + + _, _, err = serverConn.ReadFrom(buf) + assertion = assert.NoError(t, err) + allSucceed = allSucceed && assertion + assertion = assert.Equal(t, []byte(expectedString), buf) + + return allSucceed && assertion +} + func TestNewResolvedUDPConn(t *testing.T) { hostPort := "blahblah:34322" mockServer, clientConn, err := newUDPConn() - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) defer mockServer.Close() resolver := mockResolver{} @@ -106,7 +138,7 @@ func TestNewResolvedUDPConn(t *testing.T) { conn, err := newResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) - assert.NotNil(t, conn) + require.NotNil(t, conn) err = conn.Close() assert.NoError(t, err) @@ -122,9 +154,7 @@ func TestResolvedUDPConnWrites(t *testing.T) { hostPort := "blahblah:34322" mockServer, clientConn, err := newUDPConn() - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) defer mockServer.Close() resolver := mockResolver{} @@ -147,18 +177,9 @@ func TestResolvedUDPConnWrites(t *testing.T) { conn, err := newResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) - assert.NotNil(t, conn) + require.NotNil(t, conn) - expectedString := "yo this is a test" - _, err = conn.Write([]byte(expectedString)) - assert.NoError(t, err) - - var buf = make([]byte, len(expectedString)) - err = mockServer.SetReadDeadline(time.Now().Add(time.Second)) - assert.NoError(t, err) - _, _, err = mockServer.ReadFrom(buf) - assert.NoError(t, err) - assert.Equal(t, []byte(expectedString), buf) + assertConnWritable(t, conn, mockServer) err = conn.Close() assert.NoError(t, err) @@ -174,23 +195,18 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { hostPort := "blahblah:34322" mockServer, clientConn, err := newUDPConn() - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) defer mockServer.Close() resolver := mockResolver{} - resolveCall := resolver. + resolver. On("ResolveUDPAddr", "udp", hostPort). - Return(nil, fmt.Errorf("failed to resolve")) - - timer := time.AfterFunc(time.Millisecond*100, func() { - resolveCall.Return(&net.UDPAddr{ + Return(nil, fmt.Errorf("failed to resolve")).Once(). + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ IP: net.IPv4(123, 123, 123, 123), Port: 34322, }, nil) - }) - defer timer.Stop() dialer := mockDialer{} dialer. @@ -198,36 +214,31 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { IP: net.IPv4(123, 123, 123, 123), Port: 34322, }). - Return(clientConn, nil). - Once() + Return(clientConn, nil).Once() - conn, err := newResolvedUDPConn(hostPort, time.Millisecond*100, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) - assert.NotNil(t, conn) + require.NotNil(t, conn) - err = conn.SetWriteBuffer(65000) + err = conn.SetWriteBuffer(UDPPacketMaxLength) assert.NoError(t, err) - <-time.After(time.Millisecond * 200) - - fd, _ := clientConn.File() - bufferBytes, _ := syscall.GetsockoptInt(int(fd.Fd()), syscall.SOL_SOCKET, syscall.SO_SNDBUF) - if runtime.GOOS == "darwin" { - assert.Equal(t, 65000, bufferBytes) - } else { - assert.GreaterOrEqual(t, 65000*2, bufferBytes) + for i := 0; i < 10; i++ { + conn.connMtx.RLock() + connected := conn.conn != nil + conn.connMtx.RUnlock() + if connected { + break + } + time.Sleep(time.Millisecond * 10) } - expectedString := "yo this is a test" - _, err = conn.Write([]byte(expectedString)) - assert.NoError(t, err) + conn.connMtx.RLock() + assert.NotNil(t, conn.conn) + conn.connMtx.RUnlock() - var buf = make([]byte, len(expectedString)) - err = mockServer.SetReadDeadline(time.Now().Add(time.Second)) - assert.NoError(t, err) - _, _, err = mockServer.ReadFrom(buf) - assert.NoError(t, err) - assert.Equal(t, []byte(expectedString), buf) + assertConnWritable(t, conn, mockServer) + assertSockBufferSize(t, UDPPacketMaxLength, clientConn) err = conn.Close() assert.NoError(t, err) @@ -243,24 +254,23 @@ func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { hostPort := "blahblah:34322" mockServer, clientConn, err := newUDPConn() - if !assert.NoError(t, err) { - t.FailNow() - } + require.NoError(t, err) defer mockServer.Close() resolver := mockResolver{} - resolveCall := resolver. + resolver. On("ResolveUDPAddr", "udp", hostPort). Return(&net.UDPAddr{ IP: net.IPv4(123, 123, 123, 123), Port: 34322, - }, nil) + }, nil).Once() - // Simulate resolve fail after 100 seconds to ensure connection isn't swapped - timer := time.AfterFunc(time.Millisecond*100, func() { - resolveCall.Return(nil, fmt.Errorf("resolve failed")) - }) - defer timer.Stop() + failCalled := make(chan struct{}) + resolver.On("ResolveUDPAddr", "udp", hostPort). + Run(func(args mock.Arguments) { + close(failCalled) + }). + Return(nil, fmt.Errorf("resolve failed")) dialer := mockDialer{} dialer. @@ -268,24 +278,70 @@ func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { IP: net.IPv4(123, 123, 123, 123), Port: 34322, }). - Return(clientConn, nil). - Once() + Return(clientConn, nil).Once() - conn, err := newResolvedUDPConn(hostPort, time.Millisecond*100, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) - assert.NotNil(t, conn) + require.NotNil(t, conn) + + var wasCalled bool + // wait at most 100 milliseconds for the second call of ResolveUDPAddr that is supposed to fail + ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*100) + select { + case <-failCalled: + wasCalled = true + case <-ctx.Done(): + } + cancel() - <-time.After(time.Millisecond * 200) - expectedString := "yo this is a test" - _, err = conn.Write([]byte(expectedString)) + assert.True(t, wasCalled) + + assertConnWritable(t, conn, mockServer) + + err = conn.Close() assert.NoError(t, err) - var buf = make([]byte, len(expectedString)) - err = mockServer.SetReadDeadline(time.Now().Add(time.Second)) + // assert the actual connection was closed + assert.Error(t, clientConn.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} + +func TestResolvedUDPConnWriteRetry(t *testing.T) { + hostPort := "blahblah:34322" + + mockServer, clientConn, err := newUDPConn() + require.NoError(t, err) + defer mockServer.Close() + + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(nil, fmt.Errorf("failed to resolve")).Once(). + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil).Once() + + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil).Once() + + conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) - _, _, err = mockServer.ReadFrom(buf) + require.NotNil(t, conn) + + err = conn.SetWriteBuffer(UDPPacketMaxLength) assert.NoError(t, err) - assert.Equal(t, []byte(expectedString), buf) + + assertConnWritable(t, conn, mockServer) + assertSockBufferSize(t, UDPPacketMaxLength, clientConn) err = conn.Close() assert.NoError(t, err) diff --git a/utils/udp_client.go b/utils/udp_client.go index 40fe661e..4dfb43c1 100644 --- a/utils/udp_client.go +++ b/utils/udp_client.go @@ -55,6 +55,8 @@ type AgentClientUDPParams struct { HostPort string MaxPacketSize int Logger log.Logger + ResolveFunc resolveFunc + DialFunc dialFunc } // NewAgentClientUDPWithParams creates a client that sends spans to Jaeger Agent over UDP. @@ -67,6 +69,14 @@ func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, params.Logger = log.StdLogger } + if params.ResolveFunc == nil { + params.ResolveFunc = net.ResolveUDPAddr + } + + if params.DialFunc == nil { + params.DialFunc = net.DialUDP + } + thriftBuffer := thrift.NewTMemoryBufferLen(params.MaxPacketSize) protocolFactory := thrift.NewTCompactProtocolFactory() client := agent.NewAgentClientFactory(thriftBuffer, protocolFactory) @@ -81,18 +91,18 @@ func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, if ip := net.ParseIP(host); ip == nil { // host is hostname, setup resolver loop in case host record changes during operation - connUDP, err = newResolvedUDPConn(params.HostPort, time.Second*30, net.ResolveUDPAddr, net.DialUDP, params.Logger) + connUDP, err = newResolvedUDPConn(params.HostPort, time.Second*30, params.ResolveFunc, params.DialFunc, params.Logger) if err != nil { return nil, err } } else { // have to use resolve even though we know hostPort contains a literal ip, in case address is ipv6 with a zone - destAddr, err := net.ResolveUDPAddr("udp", params.HostPort) + destAddr, err := params.ResolveFunc("udp", params.HostPort) if err != nil { return nil, err } - connUDP, err = net.DialUDP(destAddr.Network(), nil, destAddr) + connUDP, err = params.DialFunc(destAddr.Network(), nil, destAddr) if err != nil { return nil, err } diff --git a/utils/udp_client_test.go b/utils/udp_client_test.go new file mode 100644 index 00000000..793f8f46 --- /dev/null +++ b/utils/udp_client_test.go @@ -0,0 +1,168 @@ +package utils + +import ( + "fmt" + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber/jaeger-client-go/log" + "github.com/uber/jaeger-client-go/thrift-gen/zipkincore" +) + +func TestNewAgentClientUDPWithParamsBadHostport(t *testing.T) { + hostPort := "blahblah" + + agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ + HostPort: hostPort, + }) + + assert.Error(t, err) + assert.Nil(t, agentClient) +} + +func TestNewAgentClientUDPWithParams(t *testing.T) { + hostPort := "blahblah:34322" + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil). + Once() + + mockServer, clientConn, err := newUDPConn() + require.NoError(t, err) + defer mockServer.Close() + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil). + Once() + + agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ + HostPort: hostPort, + MaxPacketSize: 25000, + Logger: log.NullLogger, + ResolveFunc: resolver.ResolveUDPAddr, + DialFunc: dialer.DialUDP, + }) + assert.NoError(t, err) + assert.NotNil(t, agentClient) + assert.Equal(t, 25000, agentClient.maxPacketSize) + + if assert.IsType(t, &resolvedUDPConn{}, agentClient.connUDP) { + assert.Equal(t, log.NullLogger, agentClient.connUDP.(*resolvedUDPConn).logger) + } + + assertSockBufferSize(t, 25000, clientConn) + assertConnWritable(t, clientConn, mockServer) + + assert.NoError(t, agentClient.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} + +func TestNewAgentClientUDPWithParamsDefaults(t *testing.T) { + mockServer, clientConn, err := newUDPConn() + clientConn.Close() + require.NoError(t, err) + defer mockServer.Close() + + agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ + HostPort: mockServer.LocalAddr().String(), + Logger: log.NullLogger, + }) + assert.NoError(t, err) + assert.NotNil(t, agentClient) + assert.Equal(t, UDPPacketMaxLength, agentClient.maxPacketSize) + + if assert.IsType(t, &net.UDPConn{}, agentClient.connUDP) { + conn := agentClient.connUDP.(*net.UDPConn) + assertSockBufferSize(t, UDPPacketMaxLength, conn) + assertConnWritable(t, conn, mockServer) + } + + assert.NoError(t, agentClient.Close()) +} + +func TestNewAgentClientUDPWithParamsIPHost(t *testing.T) { + hostPort := "123.123.123.123:34322" + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil). + Once() + + mockServer, clientConn, err := newUDPConn() + require.NoError(t, err) + defer mockServer.Close() + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil). + Once() + + agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ + HostPort: hostPort, + MaxPacketSize: 25000, + Logger: log.NullLogger, + ResolveFunc: resolver.ResolveUDPAddr, + DialFunc: dialer.DialUDP, + }) + assert.NoError(t, err) + assert.NotNil(t, agentClient) + assert.Equal(t, 25000, agentClient.maxPacketSize) + + assert.IsType(t, &net.UDPConn{}, agentClient.connUDP) + + assertSockBufferSize(t, 25000, clientConn) + assertConnWritable(t, clientConn, mockServer) + + assert.NoError(t, agentClient.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} + +func TestNewAgentClientUDPWithParamsIPHostResolveFails(t *testing.T) { + hostPort := "123.123.123.123:34322" + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(nil, fmt.Errorf("resolve failed")). + Once() + + dialer := mockDialer{} + + agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ + HostPort: hostPort, + MaxPacketSize: 25000, + Logger: log.NullLogger, + ResolveFunc: resolver.ResolveUDPAddr, + DialFunc: dialer.DialUDP, + }) + assert.Error(t, err) + assert.Nil(t, agentClient) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} + +func TestAgentClientUDPNotSupportZipkin(t *testing.T) { + agentClient := AgentClientUDP{} + + assert.Error(t, agentClient.EmitZipkinBatch([]*zipkincore.Span{{Name: "fakespan"}})) +} From 8ea3b31f93751ad5f227ddeb6f5deb4d238fc47f Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Thu, 2 Jul 2020 23:03:55 -0400 Subject: [PATCH 10/25] Run make fmt Signed-off-by: Trevor Foster --- utils/udp_client_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/utils/udp_client_test.go b/utils/udp_client_test.go index 793f8f46..47d74f11 100644 --- a/utils/udp_client_test.go +++ b/utils/udp_client_test.go @@ -1,3 +1,17 @@ +// Copyright (c) 2020 The Jaeger Authors. +// +// 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. + package utils import ( From d4f29aa1a1df118ab85d22aa0ac57468343aa85e Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Thu, 2 Jul 2020 23:20:21 -0400 Subject: [PATCH 11/25] Remove unused struct field Signed-off-by: Trevor Foster --- utils/resolved_udp_conn_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index 0037b1ce..1f6f2f74 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -30,7 +30,6 @@ import ( ) type mockResolver struct { - MockHost string mock.Mock } From fd62566e83f3f4b09df73889c7b270e3437f5f24 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Thu, 2 Jul 2020 23:26:17 -0400 Subject: [PATCH 12/25] Fix lint error Signed-off-by: Trevor Foster --- utils/resolved_udp_conn_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index 1f6f2f74..61ea3a0e 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -87,9 +87,9 @@ func assertSockBufferSize(t *testing.T, expectedBytes int, conn *net.UDPConn) bo // https://linux.die.net/man/7/socket if runtime.GOOS == "linux" { return assert.GreaterOrEqual(t, expectedBytes*2, bufferBytes) - } else { - return assert.Equal(t, expectedBytes, bufferBytes) } + + return assert.Equal(t, expectedBytes, bufferBytes) } func assertConnWritable(t *testing.T, conn udpConn, serverConn net.PacketConn) (allSucceed bool) { From d70dc93a287e835dfde538bc90d4fdd1894f2b09 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Fri, 3 Jul 2020 02:10:39 -0400 Subject: [PATCH 13/25] Add test for new conn established when host record changes Signed-off-by: Trevor Foster --- utils/resolved_udp_conn_test.go | 141 ++++++++++++++++++++++++++------ 1 file changed, 114 insertions(+), 27 deletions(-) diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index 61ea3a0e..93a524b7 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -110,6 +110,41 @@ func assertConnWritable(t *testing.T, conn udpConn, serverConn net.PacketConn) ( return allSucceed && assertion } +func waitForCallWithTimeout(call *mock.Call) bool { + called := make(chan struct{}) + call.Run(func(args mock.Arguments) { + close(called) + }) + + var wasCalled bool + // wait at most 100 milliseconds for the second call of ResolveUDPAddr that is supposed to fail + ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*100) + select { + case <-called: + wasCalled = true + case <-ctx.Done(): + fmt.Println("timed out") + } + cancel() + + return wasCalled +} + +func waitForConnCondition(conn *resolvedUDPConn, condition func(conn *resolvedUDPConn) bool) bool { + var conditionVal bool + for i := 0; i < 10; i++ { + conn.connMtx.RLock() + conditionVal = condition(conn) + conn.connMtx.RUnlock() + if conditionVal { + break + } + time.Sleep(time.Millisecond * 10) + } + + return conditionVal +} + func TestNewResolvedUDPConn(t *testing.T) { hostPort := "blahblah:34322" @@ -208,7 +243,7 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { }, nil) dialer := mockDialer{} - dialer. + dialCall := dialer. On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ IP: net.IPv4(123, 123, 123, 123), Port: 34322, @@ -222,19 +257,14 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { err = conn.SetWriteBuffer(UDPPacketMaxLength) assert.NoError(t, err) - for i := 0; i < 10; i++ { - conn.connMtx.RLock() - connected := conn.conn != nil - conn.connMtx.RUnlock() - if connected { - break - } - time.Sleep(time.Millisecond * 10) - } + wasCalled := waitForCallWithTimeout(dialCall) + assert.True(t, wasCalled) + + connEstablished := waitForConnCondition(conn, func(conn *resolvedUDPConn) bool { + return conn.conn != nil + }) - conn.connMtx.RLock() - assert.NotNil(t, conn.conn) - conn.connMtx.RUnlock() + assert.True(t, connEstablished) assertConnWritable(t, conn, mockServer) assertSockBufferSize(t, UDPPacketMaxLength, clientConn) @@ -264,11 +294,7 @@ func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { Port: 34322, }, nil).Once() - failCalled := make(chan struct{}) - resolver.On("ResolveUDPAddr", "udp", hostPort). - Run(func(args mock.Arguments) { - close(failCalled) - }). + failCall := resolver.On("ResolveUDPAddr", "udp", hostPort). Return(nil, fmt.Errorf("resolve failed")) dialer := mockDialer{} @@ -283,15 +309,7 @@ func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { assert.NoError(t, err) require.NotNil(t, conn) - var wasCalled bool - // wait at most 100 milliseconds for the second call of ResolveUDPAddr that is supposed to fail - ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*100) - select { - case <-failCalled: - wasCalled = true - case <-ctx.Done(): - } - cancel() + wasCalled := waitForCallWithTimeout(failCall) assert.True(t, wasCalled) @@ -351,3 +369,72 @@ func TestResolvedUDPConnWriteRetry(t *testing.T) { resolver.AssertExpectations(t) dialer.AssertExpectations(t) } + +func TestResolvedUDPConnChanges(t *testing.T) { + hostPort := "blahblah:34322" + + mockServer, clientConn, err := newUDPConn() + require.NoError(t, err) + defer mockServer.Close() + + mockServer2, clientConn2, err := newUDPConn() + require.NoError(t, err) + defer mockServer2.Close() + + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }, nil).Once(). + On("ResolveUDPAddr", "udp", hostPort). + Return(&net.UDPAddr{ + IP: net.IPv4(100, 100, 100, 100), + Port: 34322, + }, nil) + + dialer := mockDialer{} + dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(123, 123, 123, 123), + Port: 34322, + }). + Return(clientConn, nil).Once() + + secondDial := dialer.On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ + IP: net.IPv4(100, 100, 100, 100), + Port: 34322, + }).Return(clientConn2, nil).Once() + + conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.StdLogger) + assert.NoError(t, err) + require.NotNil(t, conn) + + err = conn.SetWriteBuffer(UDPPacketMaxLength) + assert.NoError(t, err) + + wasCalled := waitForCallWithTimeout(secondDial) + assert.True(t, wasCalled) + + connSwapped := waitForConnCondition(conn, func(conn *resolvedUDPConn) bool { + return conn.conn == clientConn2 + }) + + assert.True(t, connSwapped) + + assertConnWritable(t, conn, mockServer2) + assertSockBufferSize(t, UDPPacketMaxLength, clientConn2) + + err = conn.Close() + assert.NoError(t, err) + + // assert the prev connection was closed + assert.Error(t, clientConn.Close()) + + // assert the actual connection was closed + assert.Error(t, clientConn2.Close()) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} From 4161d17fd6456cfb8860160c8161685dda8ec63f Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Fri, 3 Jul 2020 03:32:41 -0400 Subject: [PATCH 14/25] Fix comment typo Signed-off-by: Trevor Foster --- utils/resolved_udp_conn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/resolved_udp_conn.go b/utils/resolved_udp_conn.go index 759ae05f..18d3462a 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/resolved_udp_conn.go @@ -159,7 +159,7 @@ func (c *resolvedUDPConn) Write(b []byte) (int, error) { func (c *resolvedUDPConn) Close() error { close(c.closeChan) - // aqcuire rlock before closing conn to ensure calls to Write drain + // acquire rlock before closing conn to ensure calls to Write drain c.connMtx.RLock() defer c.connMtx.RUnlock() From 0d18d0d2ca495ef8288bcd22dbe5167cd11be39e Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Fri, 3 Jul 2020 03:47:08 -0400 Subject: [PATCH 15/25] Add test for failed write retry Signed-off-by: Trevor Foster --- utils/resolved_udp_conn_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index 93a524b7..008dc366 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -370,6 +370,34 @@ func TestResolvedUDPConnWriteRetry(t *testing.T) { dialer.AssertExpectations(t) } +func TestResolvedUDPConnWriteRetryFails(t *testing.T) { + hostPort := "blahblah:34322" + + resolver := mockResolver{} + resolver. + On("ResolveUDPAddr", "udp", hostPort). + Return(nil, fmt.Errorf("failed to resolve")).Twice() + + dialer := mockDialer{} + + conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + assert.NoError(t, err) + require.NotNil(t, conn) + + err = conn.SetWriteBuffer(UDPPacketMaxLength) + assert.NoError(t, err) + + _, err = conn.Write([]byte("yo this is a test")) + + assert.Error(t, err) + + err = conn.Close() + assert.NoError(t, err) + + resolver.AssertExpectations(t) + dialer.AssertExpectations(t) +} + func TestResolvedUDPConnChanges(t *testing.T) { hostPort := "blahblah:34322" From 044e26ac2caf5088e2ada985bf246ad8cc203de1 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Fri, 3 Jul 2020 03:56:19 -0400 Subject: [PATCH 16/25] Add test calling NewAgentClientUDP Signed-off-by: Trevor Foster --- utils/udp_client_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/utils/udp_client_test.go b/utils/udp_client_test.go index 47d74f11..2448ab13 100644 --- a/utils/udp_client_test.go +++ b/utils/udp_client_test.go @@ -106,6 +106,26 @@ func TestNewAgentClientUDPWithParamsDefaults(t *testing.T) { assert.NoError(t, agentClient.Close()) } +func TestNewAgentClientUDPDefaults(t *testing.T) { + mockServer, clientConn, err := newUDPConn() + clientConn.Close() + require.NoError(t, err) + defer mockServer.Close() + + agentClient, err := NewAgentClientUDP(mockServer.LocalAddr().String(), 25000) + assert.NoError(t, err) + assert.NotNil(t, agentClient) + assert.Equal(t, 25000, agentClient.maxPacketSize) + + if assert.IsType(t, &net.UDPConn{}, agentClient.connUDP) { + conn := agentClient.connUDP.(*net.UDPConn) + assertSockBufferSize(t, 25000, conn) + assertConnWritable(t, conn, mockServer) + } + + assert.NoError(t, agentClient.Close()) +} + func TestNewAgentClientUDPWithParamsIPHost(t *testing.T) { hostPort := "123.123.123.123:34322" resolver := mockResolver{} From 9c36339b24117a35b23160f2ef08d97a94920731 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Fri, 3 Jul 2020 04:05:01 -0400 Subject: [PATCH 17/25] Remove sleep on last try evaluating connection condition Signed-off-by: Trevor Foster --- utils/resolved_udp_conn_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/utils/resolved_udp_conn_test.go b/utils/resolved_udp_conn_test.go index 008dc366..3f2f578c 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/resolved_udp_conn_test.go @@ -136,9 +136,10 @@ func waitForConnCondition(conn *resolvedUDPConn, condition func(conn *resolvedUD conn.connMtx.RLock() conditionVal = condition(conn) conn.connMtx.RUnlock() - if conditionVal { + if conditionVal || i >= 9 { break } + time.Sleep(time.Millisecond * 10) } From 70bff5e4aabe05e7c7abf8ff9fc633d7515231e1 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Tue, 7 Jul 2020 02:02:45 -0400 Subject: [PATCH 18/25] Rename resolved udp conn to reconnecting udp conn, add opt-out option for reconnecting client and option for reconnect interval Signed-off-by: Trevor Foster --- README.md | 2 + config/config.go | 20 ++- config/config_env.go | 61 ++++++--- constants.go | 6 - transport_udp.go | 25 +--- ...d_udp_conn.go => reconnecting_udp_conn.go} | 34 ++--- ..._test.go => reconnecting_udp_conn_test.go} | 30 +++-- utils/udp_client.go | 56 ++++---- utils/udp_client_test.go | 124 +++--------------- 9 files changed, 154 insertions(+), 204 deletions(-) rename utils/{resolved_udp_conn.go => reconnecting_udp_conn.go} (77%) rename utils/{resolved_udp_conn_test.go => reconnecting_udp_conn_test.go} (88%) diff --git a/README.md b/README.md index e7b13b1c..687f5780 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,8 @@ JAEGER_PASSWORD | Password to send as part of "Basic" authentication to the coll JAEGER_REPORTER_LOG_SPANS | Whether the reporter should also log the spans" `true` or `false` (default `false`). JAEGER_REPORTER_MAX_QUEUE_SIZE | The reporter's maximum queue size (default `100`). JAEGER_REPORTER_FLUSH_INTERVAL | The reporter's flush interval, with units, e.g. `500ms` or `2s` ([valid units][timeunits]; default `1s`). +JAEGER_REPORTER_ATTEMPT_RECONNECTING_DISABLED | When true, disables udp connection helper that periodically re-resolves the agent's hostname and reconnects if there was a change (default `false`). +JAEGER_REPORTER_ATTEMPT_RECONNECT_INTERVAL | Controls how often the agent client re-resolves the provided hostname in order to detect address changes ([valid units][timeunits]; default `30s`). JAEGER_SAMPLER_TYPE | The sampler type: `remote`, `const`, `probabilistic`, `ratelimiting` (default `remote`). See also https://www.jaegertracing.io/docs/latest/sampling/. JAEGER_SAMPLER_PARAM | The sampler parameter (number). JAEGER_SAMPLER_MANAGER_HOST_PORT | (deprecated) The HTTP endpoint when using the `remote` sampler. diff --git a/config/config.go b/config/config.go index 866af6bf..bb122829 100644 --- a/config/config.go +++ b/config/config.go @@ -22,6 +22,7 @@ import ( "time" "github.com/opentracing/opentracing-go" + "github.com/uber/jaeger-client-go/utils" "github.com/uber/jaeger-client-go" "github.com/uber/jaeger-client-go/internal/baggage/remote" @@ -124,6 +125,17 @@ type ReporterConfig struct { // Can be provided by FromEnv() via the environment variable named JAEGER_AGENT_HOST / JAEGER_AGENT_PORT LocalAgentHostPort string `yaml:"localAgentHostPort"` + // DisableAttemptReconnecting when true, disables udp connection helper that periodically re-resolves + // the agent's hostname and reconnects if there was a change. This option only + // applies if LocalAgentHostPort is specified. + // Can be provided by FromEnv() via the environment variable named JAEGER_REPORTER_ATTEMPT_RECONNECTING_DISABLED + DisableAttemptReconnecting bool `yaml:"disableAttemptReconnecting"` + + // AttemptReconnectInterval controls how often the agent client re-resolves the provided hostname + // in order to detect address changes. This option only applies if DisableAttemptReconnecting is false. + // Can be provided by FromEnv() via the environment variable named JAEGER_REPORTER_ATTEMPT_RECONNECT_INTERVAL + AttemptReconnectInterval time.Duration + // CollectorEndpoint instructs reporter to send spans to jaeger-collector at this URL. // Can be provided by FromEnv() via the environment variable named JAEGER_ENDPOINT CollectorEndpoint string `yaml:"collectorEndpoint"` @@ -411,8 +423,12 @@ func (rc *ReporterConfig) newTransport(logger jaeger.Logger) (jaeger.Transport, return transport.NewHTTPTransport(rc.CollectorEndpoint, httpOptions...), nil default: return jaeger.NewUDPTransportWithParams(jaeger.UDPTransportParams{ - HostPort: rc.LocalAgentHostPort, - Logger: logger, + AgentClientUDPParams: utils.AgentClientUDPParams{ + HostPort: rc.LocalAgentHostPort, + Logger: logger, + DisableAttemptReconnecting: rc.DisableAttemptReconnecting, + AttemptReconnectInterval: rc.AttemptReconnectInterval, + }, }) } } diff --git a/config/config_env.go b/config/config_env.go index f38eb9d9..e510c936 100644 --- a/config/config_env.go +++ b/config/config_env.go @@ -24,30 +24,33 @@ import ( "github.com/opentracing/opentracing-go" "github.com/pkg/errors" + "github.com/uber/jaeger-client-go/utils" "github.com/uber/jaeger-client-go" ) const ( // environment variable names - envServiceName = "JAEGER_SERVICE_NAME" - envDisabled = "JAEGER_DISABLED" - envRPCMetrics = "JAEGER_RPC_METRICS" - envTags = "JAEGER_TAGS" - envSamplerType = "JAEGER_SAMPLER_TYPE" - envSamplerParam = "JAEGER_SAMPLER_PARAM" - envSamplerManagerHostPort = "JAEGER_SAMPLER_MANAGER_HOST_PORT" // Deprecated by envSamplingEndpoint - envSamplingEndpoint = "JAEGER_SAMPLING_ENDPOINT" - envSamplerMaxOperations = "JAEGER_SAMPLER_MAX_OPERATIONS" - envSamplerRefreshInterval = "JAEGER_SAMPLER_REFRESH_INTERVAL" - envReporterMaxQueueSize = "JAEGER_REPORTER_MAX_QUEUE_SIZE" - envReporterFlushInterval = "JAEGER_REPORTER_FLUSH_INTERVAL" - envReporterLogSpans = "JAEGER_REPORTER_LOG_SPANS" - envEndpoint = "JAEGER_ENDPOINT" - envUser = "JAEGER_USER" - envPassword = "JAEGER_PASSWORD" - envAgentHost = "JAEGER_AGENT_HOST" - envAgentPort = "JAEGER_AGENT_PORT" + envServiceName = "JAEGER_SERVICE_NAME" + envDisabled = "JAEGER_DISABLED" + envRPCMetrics = "JAEGER_RPC_METRICS" + envTags = "JAEGER_TAGS" + envSamplerType = "JAEGER_SAMPLER_TYPE" + envSamplerParam = "JAEGER_SAMPLER_PARAM" + envSamplerManagerHostPort = "JAEGER_SAMPLER_MANAGER_HOST_PORT" // Deprecated by envSamplingEndpoint + envSamplingEndpoint = "JAEGER_SAMPLING_ENDPOINT" + envSamplerMaxOperations = "JAEGER_SAMPLER_MAX_OPERATIONS" + envSamplerRefreshInterval = "JAEGER_SAMPLER_REFRESH_INTERVAL" + envReporterMaxQueueSize = "JAEGER_REPORTER_MAX_QUEUE_SIZE" + envReporterFlushInterval = "JAEGER_REPORTER_FLUSH_INTERVAL" + envReporterLogSpans = "JAEGER_REPORTER_LOG_SPANS" + envReporterAttemptReconnectingDisabled = "JAEGER_REPORTER_ATTEMPT_RECONNECTING_DISABLED" + envReporterAttemptReconnectInterval = "JAEGER_REPORTER_ATTEMPT_RECONNECT_INTERVAL" + envEndpoint = "JAEGER_ENDPOINT" + envUser = "JAEGER_USER" + envPassword = "JAEGER_PASSWORD" + envAgentHost = "JAEGER_AGENT_HOST" + envAgentPort = "JAEGER_AGENT_PORT" ) // FromEnv uses environment variables to set the tracer's Configuration @@ -188,13 +191,13 @@ func (rc *ReporterConfig) reporterConfigFromEnv() (*ReporterConfig, error) { rc.Password = pswd } else { useEnv := false - host := jaeger.DefaultUDPSpanServerHost + host := utils.DefaultUDPSpanServerHost if e := os.Getenv(envAgentHost); e != "" { host = e useEnv = true } - port := jaeger.DefaultUDPSpanServerPort + port := utils.DefaultUDPSpanServerPort if e := os.Getenv(envAgentPort); e != "" { if value, err := strconv.ParseInt(e, 10, 0); err == nil { port = int(value) @@ -206,6 +209,24 @@ func (rc *ReporterConfig) reporterConfigFromEnv() (*ReporterConfig, error) { if useEnv || rc.LocalAgentHostPort == "" { rc.LocalAgentHostPort = fmt.Sprintf("%s:%d", host, port) } + + if e := os.Getenv(envReporterAttemptReconnectingDisabled); e != "" { + if value, err := strconv.ParseBool(e); err == nil { + rc.DisableAttemptReconnecting = value + } else { + return nil, errors.Wrapf(err, "cannot parse env var %s=%s", envReporterAttemptReconnectingDisabled, e) + } + } + + if !rc.DisableAttemptReconnecting { + if e := os.Getenv(envReporterAttemptReconnectInterval); e != "" { + if value, err := time.ParseDuration(e); err == nil { + rc.AttemptReconnectInterval = value + } else { + return nil, errors.Wrapf(err, "cannot parse env var %s=%s", envReporterAttemptReconnectInterval, e) + } + } + } } return rc, nil diff --git a/constants.go b/constants.go index 0012a4c2..f6d41e0d 100644 --- a/constants.go +++ b/constants.go @@ -83,12 +83,6 @@ const ( // at least a fixed number of traces per second. SamplerTypeLowerBound = "lowerbound" - // DefaultUDPSpanServerHost is the default host to send the spans to, via UDP - DefaultUDPSpanServerHost = "localhost" - - // DefaultUDPSpanServerPort is the default port to send the spans to, via UDP - DefaultUDPSpanServerPort = 6831 - // DefaultSamplingServerPort is the default port to fetch sampling config from, via http DefaultSamplingServerPort = 5778 diff --git a/transport_udp.go b/transport_udp.go index fd57e308..f99d82ea 100644 --- a/transport_udp.go +++ b/transport_udp.go @@ -16,7 +16,6 @@ package jaeger import ( "errors" - "fmt" "github.com/uber/jaeger-client-go/internal/reporterstats" "github.com/uber/jaeger-client-go/log" @@ -61,22 +60,12 @@ type udpSender struct { // UDPTransportParams allows specifying options for initializing a UDPTransport. An instance of this struct should // be passed to NewUDPTransportWithParams. type UDPTransportParams struct { - HostPort string - MaxPacketSize int - Logger log.Logger + utils.AgentClientUDPParams } // NewUDPTransportWithParams creates a reporter that submits spans to jaeger-agent. // TODO: (breaking change) move to transport/ package. func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { - if len(params.HostPort) == 0 { - params.HostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) - } - - if params.MaxPacketSize == 0 { - params.MaxPacketSize = utils.UDPPacketMaxLength - } - if params.Logger == nil { params.Logger = log.StdLogger } @@ -87,11 +76,7 @@ func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { thriftBuffer := thrift.NewTMemoryBufferLen(params.MaxPacketSize) thriftProtocol := protocolFactory.GetProtocol(thriftBuffer) - client, err := utils.NewAgentClientUDPWithParams(utils.AgentClientUDPParams{ - HostPort: params.HostPort, - MaxPacketSize: params.MaxPacketSize, - Logger: params.Logger, - }) + client, err := utils.NewAgentClientUDPWithParams(params.AgentClientUDPParams) if err != nil { return nil, err } @@ -108,8 +93,10 @@ func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { // TODO: (breaking change) move to transport/ package. func NewUDPTransport(hostPort string, maxPacketSize int) (Transport, error) { return NewUDPTransportWithParams(UDPTransportParams{ - HostPort: hostPort, - MaxPacketSize: maxPacketSize, + AgentClientUDPParams: utils.AgentClientUDPParams{ + HostPort: hostPort, + MaxPacketSize: maxPacketSize, + }, }) } diff --git a/utils/resolved_udp_conn.go b/utils/reconnecting_udp_conn.go similarity index 77% rename from utils/resolved_udp_conn.go rename to utils/reconnecting_udp_conn.go index 18d3462a..0dffc7fa 100644 --- a/utils/resolved_udp_conn.go +++ b/utils/reconnecting_udp_conn.go @@ -24,9 +24,9 @@ import ( "github.com/uber/jaeger-client-go/log" ) -// resolvedUDPConn is an implementation of udpConn that continually resolves the provided hostname and atomically swaps -// the connection if the dial succeeds -type resolvedUDPConn struct { +// reconnectingUDPConn is an implementation of udpConn that resolves hostPort every resolveTimeout, if the resolved address is +// different than the current conn then the new address is dialed and the conn is swapped. +type reconnectingUDPConn struct { hostPort string resolveFunc resolveFunc dialFunc dialFunc @@ -42,10 +42,10 @@ type resolvedUDPConn struct { type resolveFunc func(network string, hostPort string) (*net.UDPAddr, error) type dialFunc func(network string, laddr, raddr *net.UDPAddr) (*net.UDPConn, error) -// newResolvedUDPConn returns a new udpConn that resolves hostPort every resolveTimeout, if the resolved address is +// newReconnectingUDPConn returns a new udpConn that resolves hostPort every resolveTimeout, if the resolved address is // different than the current conn then the new address is dialed and the conn is swapped. -func newResolvedUDPConn(hostPort string, resolveTimeout time.Duration, resolveFunc resolveFunc, dialFunc dialFunc, logger log.Logger) (*resolvedUDPConn, error) { - conn := &resolvedUDPConn{ +func newReconnectingUDPConn(hostPort string, resolveTimeout time.Duration, resolveFunc resolveFunc, dialFunc dialFunc, logger log.Logger) (*reconnectingUDPConn, error) { + conn := &reconnectingUDPConn{ hostPort: hostPort, resolveFunc: resolveFunc, dialFunc: dialFunc, @@ -57,12 +57,12 @@ func newResolvedUDPConn(hostPort string, resolveTimeout time.Duration, resolveFu logger.Error(fmt.Sprintf("failed resolving destination address on connection startup, with err: %q. retrying in %s", err.Error(), resolveTimeout)) } - go conn.resolveLoop(resolveTimeout) + go conn.reconnectLoop(resolveTimeout) return conn, nil } -func (c *resolvedUDPConn) resolveLoop(resolveTimeout time.Duration) { +func (c *reconnectingUDPConn) reconnectLoop(resolveTimeout time.Duration) { ticker := time.NewTicker(resolveTimeout) defer ticker.Stop() @@ -78,7 +78,7 @@ func (c *resolvedUDPConn) resolveLoop(resolveTimeout time.Duration) { } } -func (c *resolvedUDPConn) attemptResolveAndDial() error { +func (c *reconnectingUDPConn) attemptResolveAndDial() error { newAddr, err := c.resolveFunc("udp", c.hostPort) if err != nil { return fmt.Errorf("failed to resolve new addr for host %q, with err: %w", c.hostPort, err) @@ -100,7 +100,7 @@ func (c *resolvedUDPConn) attemptResolveAndDial() error { return nil } -func (c *resolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { +func (c *reconnectingUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { connUDP, err := c.dialFunc(newAddr.Network(), nil, newAddr) if err != nil { return err @@ -127,7 +127,7 @@ func (c *resolvedUDPConn) attemptDialNewAddr(newAddr *net.UDPAddr) error { } // Write calls net.udpConn.Write, if it fails an attempt is made to connect to a new addr, if that succeeds the write is retried before returning -func (c *resolvedUDPConn) Write(b []byte) (int, error) { +func (c *reconnectingUDPConn) Write(b []byte) (int, error) { var bytesWritten int var err error @@ -155,13 +155,13 @@ func (c *resolvedUDPConn) Write(b []byte) (int, error) { return bytesWritten, err } -// Close stops the resolveLoop, then closes the connection via net.udpConn 's implementation -func (c *resolvedUDPConn) Close() error { +// Close stops the reconnectLoop, then closes the connection via net.udpConn 's implementation +func (c *reconnectingUDPConn) Close() error { close(c.closeChan) - // acquire rlock before closing conn to ensure calls to Write drain - c.connMtx.RLock() - defer c.connMtx.RUnlock() + // acquire rw lock before closing conn to ensure calls to Write drain + c.connMtx.Lock() + defer c.connMtx.Unlock() if c.conn != nil { return c.conn.Close() @@ -172,7 +172,7 @@ func (c *resolvedUDPConn) Close() error { // SetWriteBuffer defers to the net.udpConn SetWriteBuffer implementation wrapped with a RLock. if no conn is currently held // and SetWriteBuffer is called store bufferBytes to be set for new conns -func (c *resolvedUDPConn) SetWriteBuffer(bytes int) error { +func (c *reconnectingUDPConn) SetWriteBuffer(bytes int) error { var err error c.connMtx.RLock() diff --git a/utils/resolved_udp_conn_test.go b/utils/reconnecting_udp_conn_test.go similarity index 88% rename from utils/resolved_udp_conn_test.go rename to utils/reconnecting_udp_conn_test.go index 3f2f578c..3d42ae59 100644 --- a/utils/resolved_udp_conn_test.go +++ b/utils/reconnecting_udp_conn_test.go @@ -58,8 +58,16 @@ func (m *mockDialer) DialUDP(network string, laddr, raddr *net.UDPAddr) (*net.UD return a0.(*net.UDPConn), args.Error(1) } +func newUDPListener() (net.PacketConn, error) { + return net.ListenPacket("udp", "127.0.0.1:0") +} + +func newUDPListenerOnPort(port int) (net.PacketConn, error) { + return net.ListenPacket("udp", fmt.Sprintf("127.0.0.1:%d", port)) +} + func newUDPConn() (net.PacketConn, *net.UDPConn, error) { - mockServer, err := net.ListenPacket("udp", "127.0.0.1:0") + mockServer, err := newUDPListener() if err != nil { return nil, nil, err } @@ -130,7 +138,7 @@ func waitForCallWithTimeout(call *mock.Call) bool { return wasCalled } -func waitForConnCondition(conn *resolvedUDPConn, condition func(conn *resolvedUDPConn) bool) bool { +func waitForConnCondition(conn *reconnectingUDPConn, condition func(conn *reconnectingUDPConn) bool) bool { var conditionVal bool for i := 0; i < 10; i++ { conn.connMtx.RLock() @@ -171,7 +179,7 @@ func TestNewResolvedUDPConn(t *testing.T) { Return(clientConn, nil). Once() - conn, err := newResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) require.NotNil(t, conn) @@ -210,7 +218,7 @@ func TestResolvedUDPConnWrites(t *testing.T) { Return(clientConn, nil). Once() - conn, err := newResolvedUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Hour, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) require.NotNil(t, conn) @@ -251,7 +259,7 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { }). Return(clientConn, nil).Once() - conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) require.NotNil(t, conn) @@ -261,7 +269,7 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { wasCalled := waitForCallWithTimeout(dialCall) assert.True(t, wasCalled) - connEstablished := waitForConnCondition(conn, func(conn *resolvedUDPConn) bool { + connEstablished := waitForConnCondition(conn, func(conn *reconnectingUDPConn) bool { return conn.conn != nil }) @@ -306,7 +314,7 @@ func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { }). Return(clientConn, nil).Once() - conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) require.NotNil(t, conn) @@ -351,7 +359,7 @@ func TestResolvedUDPConnWriteRetry(t *testing.T) { }). Return(clientConn, nil).Once() - conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) require.NotNil(t, conn) @@ -381,7 +389,7 @@ func TestResolvedUDPConnWriteRetryFails(t *testing.T) { dialer := mockDialer{} - conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) require.NotNil(t, conn) @@ -436,7 +444,7 @@ func TestResolvedUDPConnChanges(t *testing.T) { Port: 34322, }).Return(clientConn2, nil).Once() - conn, err := newResolvedUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.StdLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.StdLogger) assert.NoError(t, err) require.NotNil(t, conn) @@ -446,7 +454,7 @@ func TestResolvedUDPConnChanges(t *testing.T) { wasCalled := waitForCallWithTimeout(secondDial) assert.True(t, wasCalled) - connSwapped := waitForConnCondition(conn, func(conn *resolvedUDPConn) bool { + connSwapped := waitForConnCondition(conn, func(conn *reconnectingUDPConn) bool { return conn.conn == clientConn2 }) diff --git a/utils/udp_client.go b/utils/udp_client.go index 4dfb43c1..3ac939bb 100644 --- a/utils/udp_client.go +++ b/utils/udp_client.go @@ -49,18 +49,35 @@ type udpConn interface { Close() error } +const ( + // DefaultUDPSpanServerHost is the default host to send the spans to, via UDP + DefaultUDPSpanServerHost = "localhost" + + // DefaultUDPSpanServerPort is the default port to send the spans to, via UDP + DefaultUDPSpanServerPort = 6831 +) + // AgentClientUDPParams allows specifying options for initializing an AgentClientUDP. An instance of this struct should // be passed to NewAgentClientUDPWithParams. type AgentClientUDPParams struct { - HostPort string - MaxPacketSize int - Logger log.Logger - ResolveFunc resolveFunc - DialFunc dialFunc + HostPort string + MaxPacketSize int + Logger log.Logger + DisableAttemptReconnecting bool + AttemptReconnectInterval time.Duration } // NewAgentClientUDPWithParams creates a client that sends spans to Jaeger Agent over UDP. func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, error) { + if len(params.HostPort) == 0 { + params.HostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) + } + + // validate hostport + if _, _, err := net.SplitHostPort(params.HostPort); err != nil { + return nil, err + } + if params.MaxPacketSize == 0 { params.MaxPacketSize = UDPPacketMaxLength } @@ -69,12 +86,8 @@ func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, params.Logger = log.StdLogger } - if params.ResolveFunc == nil { - params.ResolveFunc = net.ResolveUDPAddr - } - - if params.DialFunc == nil { - params.DialFunc = net.DialUDP + if !params.DisableAttemptReconnecting && params.AttemptReconnectInterval == 0 { + params.AttemptReconnectInterval = time.Second * 30 } thriftBuffer := thrift.NewTMemoryBufferLen(params.MaxPacketSize) @@ -84,25 +97,20 @@ func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, var connUDP udpConn var err error - host, _, err := net.SplitHostPort(params.HostPort) - if err != nil { - return nil, err - } - - if ip := net.ParseIP(host); ip == nil { - // host is hostname, setup resolver loop in case host record changes during operation - connUDP, err = newResolvedUDPConn(params.HostPort, time.Second*30, params.ResolveFunc, params.DialFunc, params.Logger) + if params.DisableAttemptReconnecting { + // have to use resolve even though we know hostPort contains a literal ip, in case address is ipv6 with a zone + destAddr, err := net.ResolveUDPAddr("udp", params.HostPort) if err != nil { return nil, err } - } else { - // have to use resolve even though we know hostPort contains a literal ip, in case address is ipv6 with a zone - destAddr, err := params.ResolveFunc("udp", params.HostPort) + + connUDP, err = net.DialUDP(destAddr.Network(), nil, destAddr) if err != nil { return nil, err } - - connUDP, err = params.DialFunc(destAddr.Network(), nil, destAddr) + } else { + // host is hostname, setup resolver loop in case host record changes during operation + connUDP, err = newReconnectingUDPConn(params.HostPort, params.AttemptReconnectInterval, net.ResolveUDPAddr, net.DialUDP, params.Logger) if err != nil { return nil, err } diff --git a/utils/udp_client_test.go b/utils/udp_client_test.go index 2448ab13..fa8d1fe6 100644 --- a/utils/udp_client_test.go +++ b/utils/udp_client_test.go @@ -15,7 +15,6 @@ package utils import ( - "fmt" "net" "testing" @@ -37,162 +36,77 @@ func TestNewAgentClientUDPWithParamsBadHostport(t *testing.T) { } func TestNewAgentClientUDPWithParams(t *testing.T) { - hostPort := "blahblah:34322" - resolver := mockResolver{} - resolver. - On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil). - Once() - - mockServer, clientConn, err := newUDPConn() + mockServer, err := newUDPListener() require.NoError(t, err) defer mockServer.Close() - dialer := mockDialer{} - dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). - Return(clientConn, nil). - Once() agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ - HostPort: hostPort, + HostPort: mockServer.LocalAddr().String(), MaxPacketSize: 25000, Logger: log.NullLogger, - ResolveFunc: resolver.ResolveUDPAddr, - DialFunc: dialer.DialUDP, }) assert.NoError(t, err) assert.NotNil(t, agentClient) assert.Equal(t, 25000, agentClient.maxPacketSize) - if assert.IsType(t, &resolvedUDPConn{}, agentClient.connUDP) { - assert.Equal(t, log.NullLogger, agentClient.connUDP.(*resolvedUDPConn).logger) + if assert.IsType(t, &reconnectingUDPConn{}, agentClient.connUDP) { + assert.Equal(t, log.NullLogger, agentClient.connUDP.(*reconnectingUDPConn).logger) } - assertSockBufferSize(t, 25000, clientConn) - assertConnWritable(t, clientConn, mockServer) - assert.NoError(t, agentClient.Close()) - - resolver.AssertExpectations(t) - dialer.AssertExpectations(t) } func TestNewAgentClientUDPWithParamsDefaults(t *testing.T) { - mockServer, clientConn, err := newUDPConn() - clientConn.Close() + mockServer, err := newUDPListenerOnPort(DefaultUDPSpanServerPort) require.NoError(t, err) defer mockServer.Close() - agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ - HostPort: mockServer.LocalAddr().String(), - Logger: log.NullLogger, - }) + agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{}) assert.NoError(t, err) assert.NotNil(t, agentClient) assert.Equal(t, UDPPacketMaxLength, agentClient.maxPacketSize) - if assert.IsType(t, &net.UDPConn{}, agentClient.connUDP) { - conn := agentClient.connUDP.(*net.UDPConn) - assertSockBufferSize(t, UDPPacketMaxLength, conn) - assertConnWritable(t, conn, mockServer) + if assert.IsType(t, &reconnectingUDPConn{}, agentClient.connUDP) { + assert.Equal(t, agentClient.connUDP.(*reconnectingUDPConn).logger, log.StdLogger) } assert.NoError(t, agentClient.Close()) } func TestNewAgentClientUDPDefaults(t *testing.T) { - mockServer, clientConn, err := newUDPConn() - clientConn.Close() + mockServer, err := newUDPListenerOnPort(DefaultUDPSpanServerPort) require.NoError(t, err) defer mockServer.Close() - agentClient, err := NewAgentClientUDP(mockServer.LocalAddr().String(), 25000) + agentClient, err := NewAgentClientUDP(mockServer.LocalAddr().String(), 0) assert.NoError(t, err) assert.NotNil(t, agentClient) - assert.Equal(t, 25000, agentClient.maxPacketSize) + assert.Equal(t, UDPPacketMaxLength, agentClient.maxPacketSize) - if assert.IsType(t, &net.UDPConn{}, agentClient.connUDP) { - conn := agentClient.connUDP.(*net.UDPConn) - assertSockBufferSize(t, 25000, conn) - assertConnWritable(t, conn, mockServer) + if assert.IsType(t, &reconnectingUDPConn{}, agentClient.connUDP) { + assert.Equal(t, agentClient.connUDP.(*reconnectingUDPConn).logger, log.StdLogger) } assert.NoError(t, agentClient.Close()) } -func TestNewAgentClientUDPWithParamsIPHost(t *testing.T) { - hostPort := "123.123.123.123:34322" - resolver := mockResolver{} - resolver. - On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil). - Once() - - mockServer, clientConn, err := newUDPConn() +func TestNewAgentClientUDPWithParamsReconnectingDisabled(t *testing.T) { + mockServer, err := newUDPListener() require.NoError(t, err) defer mockServer.Close() - dialer := mockDialer{} - dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). - Return(clientConn, nil). - Once() agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ - HostPort: hostPort, - MaxPacketSize: 25000, - Logger: log.NullLogger, - ResolveFunc: resolver.ResolveUDPAddr, - DialFunc: dialer.DialUDP, + HostPort: mockServer.LocalAddr().String(), + Logger: log.NullLogger, + DisableAttemptReconnecting: true, }) assert.NoError(t, err) assert.NotNil(t, agentClient) - assert.Equal(t, 25000, agentClient.maxPacketSize) + assert.Equal(t, UDPPacketMaxLength, agentClient.maxPacketSize) assert.IsType(t, &net.UDPConn{}, agentClient.connUDP) - assertSockBufferSize(t, 25000, clientConn) - assertConnWritable(t, clientConn, mockServer) - assert.NoError(t, agentClient.Close()) - - resolver.AssertExpectations(t) - dialer.AssertExpectations(t) -} - -func TestNewAgentClientUDPWithParamsIPHostResolveFails(t *testing.T) { - hostPort := "123.123.123.123:34322" - resolver := mockResolver{} - resolver. - On("ResolveUDPAddr", "udp", hostPort). - Return(nil, fmt.Errorf("resolve failed")). - Once() - - dialer := mockDialer{} - - agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ - HostPort: hostPort, - MaxPacketSize: 25000, - Logger: log.NullLogger, - ResolveFunc: resolver.ResolveUDPAddr, - DialFunc: dialer.DialUDP, - }) - assert.Error(t, err) - assert.Nil(t, agentClient) - - resolver.AssertExpectations(t) - dialer.AssertExpectations(t) } func TestAgentClientUDPNotSupportZipkin(t *testing.T) { From 3aedb362949378e824ee03f25ec1490f9861ce69 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Tue, 7 Jul 2020 02:34:25 -0400 Subject: [PATCH 19/25] Remove irrelevant comment, fix transport max packet size regression Signed-off-by: Trevor Foster --- transport_udp.go | 4 ++++ utils/udp_client.go | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/transport_udp.go b/transport_udp.go index f99d82ea..ec6ac4f3 100644 --- a/transport_udp.go +++ b/transport_udp.go @@ -70,6 +70,10 @@ func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { params.Logger = log.StdLogger } + if params.MaxPacketSize == 0 { + params.MaxPacketSize = utils.UDPPacketMaxLength + } + protocolFactory := thrift.NewTCompactProtocolFactory() // Each span is first written to thriftBuffer to determine its size in bytes. diff --git a/utils/udp_client.go b/utils/udp_client.go index 3ac939bb..f00e0120 100644 --- a/utils/udp_client.go +++ b/utils/udp_client.go @@ -98,7 +98,6 @@ func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, var err error if params.DisableAttemptReconnecting { - // have to use resolve even though we know hostPort contains a literal ip, in case address is ipv6 with a zone destAddr, err := net.ResolveUDPAddr("udp", params.HostPort) if err != nil { return nil, err From f9de33b0158c1529ffedb53e9039b93e6f920e51 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Tue, 7 Jul 2020 02:53:50 -0400 Subject: [PATCH 20/25] Add panic in case the test server listen or srv fails unexpectedly Signed-off-by: Trevor Foster --- transport/http_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/transport/http_test.go b/transport/http_test.go index 3bcf1bac..13cde480 100644 --- a/transport/http_test.go +++ b/transport/http_test.go @@ -167,7 +167,9 @@ func newHTTPServer(t *testing.T) *httpServer { }) go func() { - http.ListenAndServe(":10001", nil) + if err := http.ListenAndServe(":10001", nil); err != nil && err != http.ErrServerClosed { + panic(err) + } }() return server From 19fb55760645fc8cead431d112d5c4818d071a8b Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Tue, 7 Jul 2020 13:10:13 -0400 Subject: [PATCH 21/25] Add coverage for new env vars Signed-off-by: Trevor Foster --- config/config_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config/config_test.go b/config/config_test.go index f7ae84a8..6e2904e1 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -202,6 +202,8 @@ func TestReporter(t *testing.T) { setEnv(t, envAgentPort, "6832") setEnv(t, envUser, "user") setEnv(t, envPassword, "password") + setEnv(t, envReporterAttemptReconnectingDisabled, "false") + setEnv(t, envReporterAttemptReconnectInterval, "40s") // Existing ReporterConfig data rc := ReporterConfig{ @@ -225,6 +227,8 @@ func TestReporter(t *testing.T) { assert.Equal(t, "nonlocalhost:6832", cfg.LocalAgentHostPort) assert.Equal(t, "user01", cfg.User) assert.Equal(t, "password01", cfg.Password) + assert.Equal(t, false, cfg.DisableAttemptReconnecting) + assert.Equal(t, time.Second * 40, cfg.AttemptReconnectInterval) // Prepare setEnv(t, envEndpoint, "http://1.2.3.4:5678/api/traces") From 3003b3cbd4b48c5729f0a53e40b09cf29958e9bc Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Tue, 7 Jul 2020 13:44:38 -0400 Subject: [PATCH 22/25] Run make fmt Signed-off-by: Trevor Foster --- config/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/config_test.go b/config/config_test.go index 6e2904e1..6f97d0a6 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -228,7 +228,7 @@ func TestReporter(t *testing.T) { assert.Equal(t, "user01", cfg.User) assert.Equal(t, "password01", cfg.Password) assert.Equal(t, false, cfg.DisableAttemptReconnecting) - assert.Equal(t, time.Second * 40, cfg.AttemptReconnectInterval) + assert.Equal(t, time.Second*40, cfg.AttemptReconnectInterval) // Prepare setEnv(t, envEndpoint, "http://1.2.3.4:5678/api/traces") From 3e254f08104f88f8a4b1424c5cf394616127e139 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Wed, 8 Jul 2020 01:01:52 -0400 Subject: [PATCH 23/25] Add back constants, add helper for generating a mock udp addr, require all assertions of udp write to succeed Signed-off-by: Trevor Foster --- constants.go | 6 ++ transport/http_test.go | 3 +- utils/reconnecting_udp_conn_test.go | 133 ++++++++++++++-------------- 3 files changed, 75 insertions(+), 67 deletions(-) diff --git a/constants.go b/constants.go index f6d41e0d..0012a4c2 100644 --- a/constants.go +++ b/constants.go @@ -83,6 +83,12 @@ const ( // at least a fixed number of traces per second. SamplerTypeLowerBound = "lowerbound" + // DefaultUDPSpanServerHost is the default host to send the spans to, via UDP + DefaultUDPSpanServerHost = "localhost" + + // DefaultUDPSpanServerPort is the default port to send the spans to, via UDP + DefaultUDPSpanServerPort = 6831 + // DefaultSamplingServerPort is the default port to fetch sampling config from, via http DefaultSamplingServerPort = 5778 diff --git a/transport/http_test.go b/transport/http_test.go index 13cde480..b42b5f9e 100644 --- a/transport/http_test.go +++ b/transport/http_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/uber/jaeger-client-go/thrift" "github.com/uber/jaeger-client-go" @@ -168,7 +169,7 @@ func newHTTPServer(t *testing.T) *httpServer { go func() { if err := http.ListenAndServe(":10001", nil); err != nil && err != http.ErrServerClosed { - panic(err) + require.NoError(t, err) } }() diff --git a/utils/reconnecting_udp_conn_test.go b/utils/reconnecting_udp_conn_test.go index 3d42ae59..cc31b9a2 100644 --- a/utils/reconnecting_udp_conn_test.go +++ b/utils/reconnecting_udp_conn_test.go @@ -17,6 +17,7 @@ package utils import ( "context" "fmt" + "math/rand" "net" "runtime" "syscall" @@ -100,22 +101,18 @@ func assertSockBufferSize(t *testing.T, expectedBytes int, conn *net.UDPConn) bo return assert.Equal(t, expectedBytes, bufferBytes) } -func assertConnWritable(t *testing.T, conn udpConn, serverConn net.PacketConn) (allSucceed bool) { +func assertConnWritable(t *testing.T, conn udpConn, serverConn net.PacketConn) { expectedString := "yo this is a test" _, err := conn.Write([]byte(expectedString)) - allSucceed = assert.NoError(t, err) + require.NoError(t, err) var buf = make([]byte, len(expectedString)) err = serverConn.SetReadDeadline(time.Now().Add(time.Second)) - assertion := assert.NoError(t, err) - allSucceed = allSucceed && assertion + require.NoError(t, err) _, _, err = serverConn.ReadFrom(buf) - assertion = assert.NoError(t, err) - allSucceed = allSucceed && assertion - assertion = assert.Equal(t, []byte(expectedString), buf) - - return allSucceed && assertion + require.NoError(t, err) + require.Equal(t, []byte(expectedString), buf) } func waitForCallWithTimeout(call *mock.Call) bool { @@ -154,6 +151,20 @@ func waitForConnCondition(conn *reconnectingUDPConn, condition func(conn *reconn return conditionVal } +func newMockUDPAddr(port int) (*net.UDPAddr, error) { + var buf = make([]byte, 4) + // random is not seeded to ensure tests are deterministic (also doesnt matter if ip is valid) + _, err := rand.Read(buf) + if err != nil { + return nil, err + } + + return &net.UDPAddr{ + IP: net.IPv4(buf[0], buf[1], buf[2], buf[3]), + Port: port, + }, nil +} + func TestNewResolvedUDPConn(t *testing.T) { hostPort := "blahblah:34322" @@ -161,21 +172,18 @@ func TestNewResolvedUDPConn(t *testing.T) { require.NoError(t, err) defer mockServer.Close() + mockUDPAddr, err := newMockUDPAddr(34322) + require.NoError(t, err) + resolver := mockResolver{} resolver. On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil). + Return(mockUDPAddr, nil). Once() dialer := mockDialer{} dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). + On("DialUDP", "udp", (*net.UDPAddr)(nil), mockUDPAddr). Return(clientConn, nil). Once() @@ -200,21 +208,18 @@ func TestResolvedUDPConnWrites(t *testing.T) { require.NoError(t, err) defer mockServer.Close() + mockUDPAddr, err := newMockUDPAddr(34322) + require.NoError(t, err) + resolver := mockResolver{} resolver. On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil). + Return(mockUDPAddr, nil). Once() dialer := mockDialer{} dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). + On("DialUDP", "udp", (*net.UDPAddr)(nil), mockUDPAddr). Return(clientConn, nil). Once() @@ -241,22 +246,19 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { require.NoError(t, err) defer mockServer.Close() + mockUDPAddr, err := newMockUDPAddr(34322) + require.NoError(t, err) + resolver := mockResolver{} resolver. On("ResolveUDPAddr", "udp", hostPort). Return(nil, fmt.Errorf("failed to resolve")).Once(). On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil) + Return(mockUDPAddr, nil) dialer := mockDialer{} dialCall := dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). + On("DialUDP", "udp", (*net.UDPAddr)(nil), mockUDPAddr). Return(clientConn, nil).Once() conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) @@ -295,23 +297,20 @@ func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { require.NoError(t, err) defer mockServer.Close() + mockUDPAddr, err := newMockUDPAddr(34322) + require.NoError(t, err) + resolver := mockResolver{} resolver. On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil).Once() + Return(mockUDPAddr, nil).Once() failCall := resolver.On("ResolveUDPAddr", "udp", hostPort). Return(nil, fmt.Errorf("resolve failed")) dialer := mockDialer{} dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). + On("DialUDP", "udp", (*net.UDPAddr)(nil), mockUDPAddr). Return(clientConn, nil).Once() conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) @@ -341,22 +340,19 @@ func TestResolvedUDPConnWriteRetry(t *testing.T) { require.NoError(t, err) defer mockServer.Close() + mockUDPAddr, err := newMockUDPAddr(34322) + require.NoError(t, err) + resolver := mockResolver{} resolver. On("ResolveUDPAddr", "udp", hostPort). Return(nil, fmt.Errorf("failed to resolve")).Once(). On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil).Once() + Return(mockUDPAddr, nil).Once() dialer := mockDialer{} dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). + On("DialUDP", "udp", (*net.UDPAddr)(nil), mockUDPAddr). Return(clientConn, nil).Once() conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) @@ -414,37 +410,42 @@ func TestResolvedUDPConnChanges(t *testing.T) { require.NoError(t, err) defer mockServer.Close() + mockUDPAddr1, err := newMockUDPAddr(34322) + require.NoError(t, err) + mockServer2, clientConn2, err := newUDPConn() require.NoError(t, err) defer mockServer2.Close() + mockUDPAddr2, err := newMockUDPAddr(34322) + require.NoError(t, err) + + // ensure address doesn't duplicate mockUDPAddr1 + for i := 0; i < 10 && mockUDPAddr2.IP.Equal(mockUDPAddr1.IP); i++ { + mockUDPAddr2, err = newMockUDPAddr(34322) + require.NoError(t, err) + } + + // this is really unlikely to ever fail the test, but its here as a safeguard + require.False(t, mockUDPAddr2.IP.Equal(mockUDPAddr1.IP)) + resolver := mockResolver{} resolver. On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }, nil).Once(). + Return(mockUDPAddr1, nil).Once(). On("ResolveUDPAddr", "udp", hostPort). - Return(&net.UDPAddr{ - IP: net.IPv4(100, 100, 100, 100), - Port: 34322, - }, nil) + Return(mockUDPAddr2, nil) dialer := mockDialer{} dialer. - On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(123, 123, 123, 123), - Port: 34322, - }). + On("DialUDP", "udp", (*net.UDPAddr)(nil), mockUDPAddr1). Return(clientConn, nil).Once() - secondDial := dialer.On("DialUDP", "udp", (*net.UDPAddr)(nil), &net.UDPAddr{ - IP: net.IPv4(100, 100, 100, 100), - Port: 34322, - }).Return(clientConn2, nil).Once() + secondDial := dialer. + On("DialUDP", "udp", (*net.UDPAddr)(nil), mockUDPAddr2). + Return(clientConn2, nil).Once() - conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.StdLogger) + conn, err := newReconnectingUDPConn(hostPort, time.Millisecond*10, resolver.ResolveUDPAddr, dialer.DialUDP, log.NullLogger) assert.NoError(t, err) require.NotNil(t, conn) From 2e4ea486c6b05622c453163e2bda62ab5ef1ef41 Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Wed, 8 Jul 2020 02:12:59 -0400 Subject: [PATCH 24/25] Remove local agent constants from utils Signed-off-by: Trevor Foster --- config/config_env.go | 6 ++---- transport_udp.go | 5 +++++ utils/udp_client.go | 12 ------------ utils/udp_client_test.go | 10 ++++++---- 4 files changed, 13 insertions(+), 20 deletions(-) diff --git a/config/config_env.go b/config/config_env.go index e510c936..92d60cd5 100644 --- a/config/config_env.go +++ b/config/config_env.go @@ -24,8 +24,6 @@ import ( "github.com/opentracing/opentracing-go" "github.com/pkg/errors" - "github.com/uber/jaeger-client-go/utils" - "github.com/uber/jaeger-client-go" ) @@ -191,13 +189,13 @@ func (rc *ReporterConfig) reporterConfigFromEnv() (*ReporterConfig, error) { rc.Password = pswd } else { useEnv := false - host := utils.DefaultUDPSpanServerHost + host := jaeger.DefaultUDPSpanServerHost if e := os.Getenv(envAgentHost); e != "" { host = e useEnv = true } - port := utils.DefaultUDPSpanServerPort + port := jaeger.DefaultUDPSpanServerPort if e := os.Getenv(envAgentPort); e != "" { if value, err := strconv.ParseInt(e, 10, 0); err == nil { port = int(value) diff --git a/transport_udp.go b/transport_udp.go index ec6ac4f3..5734819a 100644 --- a/transport_udp.go +++ b/transport_udp.go @@ -16,6 +16,7 @@ package jaeger import ( "errors" + "fmt" "github.com/uber/jaeger-client-go/internal/reporterstats" "github.com/uber/jaeger-client-go/log" @@ -66,6 +67,10 @@ type UDPTransportParams struct { // NewUDPTransportWithParams creates a reporter that submits spans to jaeger-agent. // TODO: (breaking change) move to transport/ package. func NewUDPTransportWithParams(params UDPTransportParams) (Transport, error) { + if len(params.HostPort) == 0 { + params.HostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) + } + if params.Logger == nil { params.Logger = log.StdLogger } diff --git a/utils/udp_client.go b/utils/udp_client.go index f00e0120..2352643c 100644 --- a/utils/udp_client.go +++ b/utils/udp_client.go @@ -49,14 +49,6 @@ type udpConn interface { Close() error } -const ( - // DefaultUDPSpanServerHost is the default host to send the spans to, via UDP - DefaultUDPSpanServerHost = "localhost" - - // DefaultUDPSpanServerPort is the default port to send the spans to, via UDP - DefaultUDPSpanServerPort = 6831 -) - // AgentClientUDPParams allows specifying options for initializing an AgentClientUDP. An instance of this struct should // be passed to NewAgentClientUDPWithParams. type AgentClientUDPParams struct { @@ -69,10 +61,6 @@ type AgentClientUDPParams struct { // NewAgentClientUDPWithParams creates a client that sends spans to Jaeger Agent over UDP. func NewAgentClientUDPWithParams(params AgentClientUDPParams) (*AgentClientUDP, error) { - if len(params.HostPort) == 0 { - params.HostPort = fmt.Sprintf("%s:%d", DefaultUDPSpanServerHost, DefaultUDPSpanServerPort) - } - // validate hostport if _, _, err := net.SplitHostPort(params.HostPort); err != nil { return nil, err diff --git a/utils/udp_client_test.go b/utils/udp_client_test.go index fa8d1fe6..9adcfef5 100644 --- a/utils/udp_client_test.go +++ b/utils/udp_client_test.go @@ -57,11 +57,13 @@ func TestNewAgentClientUDPWithParams(t *testing.T) { } func TestNewAgentClientUDPWithParamsDefaults(t *testing.T) { - mockServer, err := newUDPListenerOnPort(DefaultUDPSpanServerPort) + mockServer, err := newUDPListenerOnPort(6831) require.NoError(t, err) defer mockServer.Close() - agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{}) + agentClient, err := NewAgentClientUDPWithParams(AgentClientUDPParams{ + HostPort: "localhost:6831", + }) assert.NoError(t, err) assert.NotNil(t, agentClient) assert.Equal(t, UDPPacketMaxLength, agentClient.maxPacketSize) @@ -74,11 +76,11 @@ func TestNewAgentClientUDPWithParamsDefaults(t *testing.T) { } func TestNewAgentClientUDPDefaults(t *testing.T) { - mockServer, err := newUDPListenerOnPort(DefaultUDPSpanServerPort) + mockServer, err := newUDPListenerOnPort(6831) require.NoError(t, err) defer mockServer.Close() - agentClient, err := NewAgentClientUDP(mockServer.LocalAddr().String(), 0) + agentClient, err := NewAgentClientUDP("localhost:6831", 0) assert.NoError(t, err) assert.NotNil(t, agentClient) assert.Equal(t, UDPPacketMaxLength, agentClient.maxPacketSize) From da03468607d5897e2ddadb2584fd54b47a890e9a Mon Sep 17 00:00:00 2001 From: Trevor Foster Date: Wed, 8 Jul 2020 20:38:31 -0400 Subject: [PATCH 25/25] Move no error requirement into mock udp addr constructor Signed-off-by: Trevor Foster --- utils/reconnecting_udp_conn_test.go | 32 ++++++++++------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/utils/reconnecting_udp_conn_test.go b/utils/reconnecting_udp_conn_test.go index cc31b9a2..62817430 100644 --- a/utils/reconnecting_udp_conn_test.go +++ b/utils/reconnecting_udp_conn_test.go @@ -151,18 +151,16 @@ func waitForConnCondition(conn *reconnectingUDPConn, condition func(conn *reconn return conditionVal } -func newMockUDPAddr(port int) (*net.UDPAddr, error) { +func newMockUDPAddr(t *testing.T, port int) *net.UDPAddr { var buf = make([]byte, 4) // random is not seeded to ensure tests are deterministic (also doesnt matter if ip is valid) _, err := rand.Read(buf) - if err != nil { - return nil, err - } + require.NoError(t, err) return &net.UDPAddr{ IP: net.IPv4(buf[0], buf[1], buf[2], buf[3]), Port: port, - }, nil + } } func TestNewResolvedUDPConn(t *testing.T) { @@ -172,8 +170,7 @@ func TestNewResolvedUDPConn(t *testing.T) { require.NoError(t, err) defer mockServer.Close() - mockUDPAddr, err := newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr := newMockUDPAddr(t, 34322) resolver := mockResolver{} resolver. @@ -208,8 +205,7 @@ func TestResolvedUDPConnWrites(t *testing.T) { require.NoError(t, err) defer mockServer.Close() - mockUDPAddr, err := newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr := newMockUDPAddr(t, 34322) resolver := mockResolver{} resolver. @@ -246,8 +242,7 @@ func TestResolvedUDPConnEventuallyDials(t *testing.T) { require.NoError(t, err) defer mockServer.Close() - mockUDPAddr, err := newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr := newMockUDPAddr(t, 34322) resolver := mockResolver{} resolver. @@ -297,8 +292,7 @@ func TestResolvedUDPConnNoSwapIfFail(t *testing.T) { require.NoError(t, err) defer mockServer.Close() - mockUDPAddr, err := newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr := newMockUDPAddr(t, 34322) resolver := mockResolver{} resolver. @@ -340,8 +334,7 @@ func TestResolvedUDPConnWriteRetry(t *testing.T) { require.NoError(t, err) defer mockServer.Close() - mockUDPAddr, err := newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr := newMockUDPAddr(t, 34322) resolver := mockResolver{} resolver. @@ -410,20 +403,17 @@ func TestResolvedUDPConnChanges(t *testing.T) { require.NoError(t, err) defer mockServer.Close() - mockUDPAddr1, err := newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr1 := newMockUDPAddr(t, 34322) mockServer2, clientConn2, err := newUDPConn() require.NoError(t, err) defer mockServer2.Close() - mockUDPAddr2, err := newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr2 := newMockUDPAddr(t, 34322) // ensure address doesn't duplicate mockUDPAddr1 for i := 0; i < 10 && mockUDPAddr2.IP.Equal(mockUDPAddr1.IP); i++ { - mockUDPAddr2, err = newMockUDPAddr(34322) - require.NoError(t, err) + mockUDPAddr2 = newMockUDPAddr(t, 34322) } // this is really unlikely to ever fail the test, but its here as a safeguard