Add resolved udp connection type, continually resolve dns names in background #520
Conversation
background Signed-off-by: Trevor Foster <[email protected]>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
| @@ -142,17 +142,62 @@ | |||
| version = "v0.0.5" | |||
|
|
|||
| [[projects]] | |||
| digest = "1:0496f0e99014b7fd0a560c539f51d0882731137b85494142f47e550e4657176a" | |||
There was a problem hiding this comment.
is there something specific required by upgrading the lock file?
There was a problem hiding this comment.
this digest changed because i use the testify/mock sub package for my tests
transport_udp.go
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Similarly for NewAgentClientUDP
There was a problem hiding this comment.
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 <[email protected]>
doubled. Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
…e exactly twice set val Signed-off-by: Trevor Foster <[email protected]>
initialization Signed-off-by: Trevor Foster <[email protected]>
utils/resolved_udp_conn_test.go
Outdated
| Port: 34322, | ||
| }). | ||
| Return(clientConn, nil). | ||
| Once() |
There was a problem hiding this comment.
nit: these seem repeated from previous test, could they be combined?
resolved, dialer := mockResolverAndDialer(hostPort, clientConn)
There was a problem hiding this comment.
i think it may be hard to combine these into a function because each test has a different set of calls being mocked
utils/resolved_udp_conn_test.go
Outdated
| On("ResolveUDPAddr", "udp", hostPort). | ||
| Return(nil, fmt.Errorf("failed to resolve")) | ||
|
|
||
| timer := time.AfterFunc(time.Millisecond*100, func() { |
There was a problem hiding this comment.
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
Outdated
| assert.GreaterOrEqual(t, 65000*2, bufferBytes) | ||
| } | ||
|
|
||
| expectedString := "yo this is a test" |
There was a problem hiding this comment.
this seems to repeat validation from the previous test, please refactor into a function
utils/udp_client.go
Outdated
| connUDP, err := net.DialUDP(destAddr.Network(), nil, destAddr) | ||
| if err != nil { | ||
| return nil, err | ||
| if ip := net.ParseIP(host); ip == nil { |
There was a problem hiding this comment.
can we add tests for this logic too? This is where the most test coverage is missing
utils/resolved_udp_conn.go
Outdated
| } | ||
|
|
||
| // 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 { |
There was a problem hiding this comment.
this part is missing test coverage. I think one of the tests is capable of reproducing this condition.
write retry Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
|
@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?). |
|
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. |
|
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 <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
all assertions of udp write to succeed Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
Signed-off-by: Trevor Foster <[email protected]>
|
Thank you @terev ! 🎉🎉🎉 |
|
@yurishkuro Will I need to make this change in the open telemetry exporter as well or will this client eventually support that interface? |
|
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. |
|
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 |
|
then it would need a similar change |
Which problem is this PR solving?
Short description of the changes