Add resolved udp connection type, continually resolve dns names in background #520

terev · 2020-06-26T07:14:05Z

Which problem is this PR solving?

Resolves UDP connection returns "write: connection refused" #393
Resolves UDP client resolve hostPort when create tracer, if IP changed but hostPort name not changed, it can not connect host. #403
Traefik doesn't reconnect to Jaeger when connection lost traefik/traefik#6093

Short description of the changes

This pr adds a new type that decorates a UDPConn
For instances where an agent address is specified as a hostname the udp transport will create an instance of this type.
This type continually attempts to resolve a new address for the host, if a new address is resolved, and the dial succeeds the currently held connection is atomically swapped with a new one

background Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

codecov · 2020-06-26T07:17:48Z

Codecov Report

Merging #520 into master will increase coverage by 0.51%.
The diff coverage is 88.74%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage   88.75%   89.27%   +0.51%     
==========================================
  Files          60       61       +1     
  Lines        3790     3915     +125     
==========================================
+ Hits         3364     3495     +131     
+ Misses        309      294      -15     
- Partials      117      126       +9

Impacted Files	Coverage Δ
config/config_env.go	`96.80% <63.63%> (-3.21%)`	⬇️
utils/udp_client.go	`65.38% <78.78%> (+65.38%)`	⬆️
utils/reconnecting_udp_conn.go	`92.50% <92.50%> (ø)`
config/config.go	`95.51% <100.00%> (+0.21%)`	⬆️
transport_udp.go	`97.59% <100.00%> (+0.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8ed773...da03468. Read the comment docs.

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro

thanks for the patch!

utils/resolved_udp_conn.go

yurishkuro · 2020-06-28T22:09:59Z

Gopkg.lock

@@ -142,17 +142,62 @@
  version = "v0.0.5"

 [[projects]]
-  digest = "1:0496f0e99014b7fd0a560c539f51d0882731137b85494142f47e550e4657176a"


is there something specific required by upgrading the lock file?

this digest changed because i use the testify/mock sub package for my tests

utils/resolved_udp_conn.go

yurishkuro · 2020-06-28T22:32:38Z

transport_udp.go

@@ -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) {


This would be a breaking change. I would rather add another function like below and delegate to it from this one:

type UDPTransportParams struct { HostPort string MaxPacketSize int Logger log.Logger } func NewUDPTransportWithParams(params UDPTransportParams)

Similarly for NewAgentClientUDP

Made this change. I'm defaulting to log.StdLogger if no logger is passed (as is the case in NewUDPTransport and NewAgentClientUDP)

…rtup Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

doubled. Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

…e exactly twice set val Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro

few more comments, thanks

transport_udp.go

utils/resolved_udp_conn.go

utils/udp_client.go

initialization Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro

almost there, most comments in the tests

transport_udp.go

utils/resolved_udp_conn.go

utils/resolved_udp_conn_test.go

yurishkuro · 2020-06-30T22:08:06Z

utils/resolved_udp_conn_test.go

+			Port: 34322,
+		}).
+		Return(clientConn, nil).
+		Once()


nit: these seem repeated from previous test, could they be combined?

resolved, dialer := mockResolverAndDialer(hostPort, clientConn)

i think it may be hard to combine these into a function because each test has a different set of calls being mocked

yurishkuro · 2020-06-30T22:15:32Z

utils/resolved_udp_conn_test.go

+		On("ResolveUDPAddr", "udp", hostPort).
+		Return(nil, fmt.Errorf("failed to resolve"))
+
+	timer := time.AfterFunc(time.Millisecond*100, func() {


hm, this could create flaky test in CI due to short time interval. Could we use something more deterministic instead, like fail the resolve on connection creation, and then do a check-sleep loop until the connection is established from the background timer loop?
e.g.

for i:=0; i < 100; i++ { lock connected := (conn.conn != nil) unlock if connected { break } time.Sleep(10ms) } lock; defer unlock assert.NotNil(t, conn.conn)

utils/resolved_udp_conn_test.go

yurishkuro · 2020-06-30T22:17:10Z

utils/resolved_udp_conn_test.go

+		assert.GreaterOrEqual(t, 65000*2, bufferBytes)
+	}
+
+	expectedString := "yo this is a test"


this seems to repeat validation from the previous test, please refactor into a function

yurishkuro · 2020-06-30T22:26:09Z

utils/udp_client.go

-	connUDP, err := net.DialUDP(destAddr.Network(), nil, destAddr)
-	if err != nil {
-		return nil, err
+	if ip := net.ParseIP(host); ip == nil {


can we add tests for this logic too? This is where the most test coverage is missing

yurishkuro · 2020-06-30T22:26:53Z

utils/resolved_udp_conn.go

+	}
+
+	// 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 {


this part is missing test coverage. I think one of the tests is capable of reproducing this condition.

https://codecov.io/gh/jaegertracing/jaeger-client-go/compare/66c008c3d6ad856cac92a0af53186efbffa8e6a5...aaf0c9b420e28648794f01cdc4c3ace3f9c74220/diff

write retry Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro

sorry, a few more comments, main one about the design from user POV.

utils/udp_client.go

utils/resolved_udp_conn.go

utils/udp_client.go

utils/resolved_udp_conn.go

terev · 2020-07-06T04:13:44Z

@yurishkuro what do you think of me changing the implementation to only re-resolve on emit instead of in the background? The re-resolve would occur before transmit and only if outside of a duration since last resolve, or on error.

My thought is this would make sure resolves only happen if something is actually being emitted. Less random dns requests in the background if there's no spans to emit.

One downside i see with this is that the dns query would extend the duration of the emit call (but probably not a lot?).

yurishkuro · 2020-07-06T04:53:37Z

We don't want to re-resolve on every emit, that's going to affect performance. I think the point of time-based re-resolve is that if the name does change then the client will reconnect within the given interval. Since there's no acks from UDP server I don't see what else can be done.

terev · 2020-07-06T05:47:33Z

Yes sorry I didn't mean every emit. Was thinking of it as if the resolved addr has a ttl, and if an emit occurs after the ttl, the reconnect is done. But yeah the resolve and syscall to create the socket could affect performance so i'll continue with the current strategy.

for reconnecting client and option for reconnect interval Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro

Look great! Thanks for sticking with it. I left just a few small comments.

transport/http_test.go

utils/reconnecting_udp_conn_test.go

constants.go

utils/reconnecting_udp_conn_test.go

all assertions of udp write to succeed Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

utils/reconnecting_udp_conn_test.go

yurishkuro

LGTM!

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro · 2020-07-09T01:14:00Z

Thank you @terev ! 🎉🎉🎉

terev · 2020-07-09T01:25:41Z

@yurishkuro Will I need to make this change in the open telemetry exporter as well or will this client eventually support that interface?

yurishkuro · 2020-07-09T01:49:12Z

I am not sure if OpenTelemetry supports UDP emission at all. The ticket for OTEL Java SDK was closed today, don't know about Go.

terev · 2020-07-09T03:34:13Z

Looks like it does still support udp emission to the agent https://github.com/open-telemetry/opentelemetry-go/blob/master/exporters/trace/jaeger/agent.go

yurishkuro · 2020-07-09T04:11:59Z

then it would need a similar change

Add resolved udp connection type, continually resolve dns names in

7f1085d

background Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

terev added 2 commits June 26, 2020 03:19

Be sure to set buffer bytes width on new connections.

3690bcb

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Lock when checking if resolved addr is new.

e7e7b32

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

terev mentioned this pull request Jun 27, 2020

Traefik doesn't reconnect to Jaeger when connection lost traefik/traefik#6093

Closed

yurishkuro reviewed Jun 28, 2020

View reviewed changes

terev added 2 commits June 29, 2020 02:56

Fixes from review comments. Dont return error if UDPConn fails on sta…

7994cdf

…rtup Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Fix failing test. Apparently the linux kernel returns the sockopt val

66b7ec4

doubled. Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

terev requested a review from yurishkuro June 29, 2020 19:31

terev added 2 commits June 29, 2020 16:30

Use atomic ops to manage bufferBytes instead of locking mutex

fc0d3eb

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Fix buffer bytes assert because sock opt value is not guaranteed to b…

c47cc4b

…e exactly twice set val Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro reviewed Jun 30, 2020

View reviewed changes

Remove intermediate init helpers, initialize close chann in struct

aaf0c9b

initialization Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro reviewed Jun 30, 2020

View reviewed changes

terev added 4 commits July 2, 2020 23:01

Fixes based on comments, more tests for udp_client.go, and test for

0264dd8

write retry Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Run make fmt

8ea3b31

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Remove unused struct field

d4f29aa

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Fix lint error

fd62566

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

terev requested a review from yurishkuro July 3, 2020 03:33

terev added 5 commits July 3, 2020 02:10

Add test for new conn established when host record changes

d70dc93

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Fix comment typo

4161d17

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Add test for failed write retry

0d18d0d

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Add test calling NewAgentClientUDP

044e26a

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Remove sleep on last try evaluating connection condition

9c36339

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro mentioned this pull request Jul 3, 2020

When restarting jaeger aegent in docker cpp libraries don't connect anymore jaegertracing/jaeger-client-cpp#204

Closed

yurishkuro reviewed Jul 5, 2020

View reviewed changes

utils/udp_client.go Outdated Show resolved Hide resolved

utils/resolved_udp_conn.go Outdated Show resolved Hide resolved

utils/udp_client.go Outdated Show resolved Hide resolved

terev commented Jul 6, 2020

View reviewed changes

utils/resolved_udp_conn.go Outdated Show resolved Hide resolved

terev added 3 commits July 7, 2020 02:02

Rename resolved udp conn to reconnecting udp conn, add opt-out option

70bff5e

for reconnecting client and option for reconnect interval Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Remove irrelevant comment, fix transport max packet size regression

3aedb36

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Add panic in case the test server listen or srv fails unexpectedly

f9de33b

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

terev requested a review from yurishkuro July 7, 2020 07:03

terev added 2 commits July 7, 2020 13:10

Add coverage for new env vars

19fb557

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

Run make fmt

3003b3c

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro reviewed Jul 8, 2020

View reviewed changes

transport/http_test.go Outdated Show resolved Hide resolved

utils/reconnecting_udp_conn_test.go Outdated Show resolved Hide resolved

constants.go Outdated Show resolved Hide resolved

utils/reconnecting_udp_conn_test.go Outdated Show resolved Hide resolved

Add back constants, add helper for generating a mock udp addr, require

3e254f0

all assertions of udp write to succeed Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

terev requested a review from yurishkuro July 8, 2020 05:22

Remove local agent constants from utils

2e4ea48

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro reviewed Jul 8, 2020

View reviewed changes

utils/reconnecting_udp_conn_test.go Outdated Show resolved Hide resolved

yurishkuro approved these changes Jul 8, 2020

View reviewed changes

Move no error requirement into mock udp addr constructor

da03468

Signed-off-by: Trevor Foster <trevor.foster@hotmail.ca>

yurishkuro merged commit 704ce28 into jaegertracing:master Jul 9, 2020

yurishkuro mentioned this pull request Jul 28, 2020

Make UdpSender lazy to be able to recover from early DNS issues jaegertracing/jaeger-client-java#726

Merged

terev mentioned this pull request Aug 16, 2020

Add reconnecting udp connection type to jaeger exporter open-telemetry/opentelemetry-go#1063

Merged

yurishkuro mentioned this pull request Nov 9, 2020

How to watch jaeger-agent disconnection jaegertracing/jaeger#2619

Closed

lopezzlaura mentioned this pull request Oct 14, 2021

Services don't resume sending spans after an agent outage jaegertracing/jaeger-client-java#808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resolved udp connection type, continually resolve dns names in background #520

Add resolved udp connection type, continually resolve dns names in background #520

terev commented Jun 26, 2020 •

edited

Loading

codecov bot commented Jun 26, 2020 •

edited

Loading

yurishkuro left a comment

yurishkuro Jun 28, 2020

terev Jun 29, 2020

yurishkuro Jun 28, 2020

yurishkuro Jun 28, 2020

terev Jun 29, 2020

yurishkuro left a comment

yurishkuro left a comment

yurishkuro Jun 30, 2020

terev Jul 3, 2020

yurishkuro Jun 30, 2020

yurishkuro Jun 30, 2020

yurishkuro Jun 30, 2020

yurishkuro Jun 30, 2020

yurishkuro left a comment

terev commented Jul 6, 2020 •

edited

Loading

yurishkuro commented Jul 6, 2020

terev commented Jul 6, 2020

yurishkuro left a comment

yurishkuro left a comment

yurishkuro commented Jul 9, 2020

terev commented Jul 9, 2020

yurishkuro commented Jul 9, 2020

terev commented Jul 9, 2020 •

edited

Loading

yurishkuro commented Jul 9, 2020

Add resolved udp connection type, continually resolve dns names in background #520

Add resolved udp connection type, continually resolve dns names in background #520

Conversation

terev commented Jun 26, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 26, 2020 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

terev commented Jul 6, 2020 • edited Loading

yurishkuro commented Jul 6, 2020

terev commented Jul 6, 2020

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Jul 9, 2020

terev commented Jul 9, 2020

yurishkuro commented Jul 9, 2020

terev commented Jul 9, 2020 • edited Loading

yurishkuro commented Jul 9, 2020

terev commented Jun 26, 2020 •

edited

Loading

codecov bot commented Jun 26, 2020 •

edited

Loading

terev commented Jul 6, 2020 •

edited

Loading

terev commented Jul 9, 2020 •

edited

Loading