Make UdpSender lazy to be able to recover from early DNS issues

[pschichtel]

this should close #369

Which problem is this PR solving?

• Issue getTracer() fails when the UDP socket hostname isn't resolvable #369

Short description of the changes

• made the UdpSender lazily create the ThriftUdpTransport instance to prevent early DNS errors from causing initialization errors. This does require locking, which has been implemented using the double-checked locking pattern to reduce the lock overhead for (usually) the majority of send calls.

[pschichtel]

Not entirely sure what would be the right action for that failing test. It seems to have relied on the UdpSender failing during initialization. If we want to keep it that way, we'd have to eagerly trigger the instantiation of the thrift tranport. I have various ways in my hand on how to do that, but I'd prefer not to unless necessary.

[pschichtel]

@yurishkuro @pavolloffay it seems like the project is a little inactive right now, or is this simply the wrong place for such a change request?

[objectiser]

@pschichtel Sorry for the delay in looking at this.

Not entirely sure what would be the right action for that failing test. It seems to have relied on the UdpSender failing during initialization. If we want to keep it that way, we'd have to eagerly trigger the instantiation of the thrift tranport. I have various ways in my hand on how to do that, but I'd prefer not to unless necessary.

I think it would be fine to change the test to check return value is UdpSender.

[yurishkuro]

I wonder if this is going to fix the more fundamental issue where the agent address may simply change, e.g. due to agent restart.

There was a recent change in the Go client that implemented periodic reconnects: jaegertracing/jaeger-client-go#520

[codecov]

Codecov Report

Merging #726 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #726   +/-   ##
=========================================
  Coverage     88.76%   88.76%           
  Complexity      596      596           
=========================================
  Files            73       73           
  Lines          2242     2242           
  Branches        289      289           
=========================================
  Hits           1990     1990           
  Misses          159      159           
  Partials         93       93           

---

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 9b0ed16...d42e192. Read the comment docs.

[pschichtel]

Updated the commit to fix the test.

@yurishkuro In its current state, this PR will not handle an address change. Once the client has successfully been created, that instance will stay the same. However adding on some kind of periodic rechecking and/or error handling in UdpSender#send should not be hard as a follow-up.

[yurishkuro]

@jpkrohling do you still think we should introduce a configuration option?

[pschichtel]

The change in that unit test makes me wonder... Would this change break some kind of fallback logic somewhere? It seems that was being tested there.

[jpkrohling]

What happens when the agent isn't available and the client keeps trying to send data? Will there be multiple stacktraces in the logs, one per batch? If so, then we definitely need to make this opt-in.

[pschichtel]

What happens when the agent isn't available and the client keeps trying to send data? Will there be multiple stacktraces in the logs, one per batch? If so, then we definitely need to make this opt-in.

What about only printing the first stacktrace or every n-th or once every n minutes ?

[jpkrohling]

First stacktrace + positive confirmation once it succeeds sounds good to me.

[pschichtel]

I incorporated the suggestions, however I'm not entirely clear on where to handle the logging. I don't think the UdpSender should just swallow the exceptions, I'd say this should be handled at a higher level. I looked at the HttpSender as well and both of them fail in the same way now with this PR: Exception on every call. So any solution should be at a common code path. Either way I'm not sure this is something that should be done in this PR.

[yurishkuro]

I expect the Reporter to catch the exceptions, since it processes items off a queue from a background thread, so there's nowhere to re-throw them.

[jpkrohling]

I looked at the HttpSender as well and both of them fail in the same way now with this PR: Exception on every call. So any solution should be at a common code path. Either way I'm not sure this is something that should be done in this PR.

Agree that this shouldn't be done in this PR. Would you please open an issue with this idea?

[pschichtel]

So I think we are done here, right?

[pschichtel]

Follow up about the logging can be found at #729