Do not create a new emitter when a shared emitter is provided

[hden]

Issue #, if available:

Currently AWSXRayRecorder's constructor will create a new emitter even when a shared emitter is specified in the AWSXRayRecorderBuilder. This creates an unnecessary Emitter / Socket for each new AWSXRayRecorder and leads to fire descriptor exhaustion if enough Emitter is created.

Description of changes:

Change AWSXRayRecorder's constructor so that it does not create a new emitter when a shared emitter is provided.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[willarmiros]

Just a quick comment, and of course would need to fix the CI. Thanks!

[willarmiros]

Not too excited about increasing our API surface for this, maybe we can make the new constructor package private?

[hden]

Sure.

[willarmiros]

@hden There's still some syntax errors it seems

[hden]

@willarmiros Oops.

[hden]

@willarmiros I've fixed the type annotations. Please take a look and tell me what you think.

[codecov-commenter]

Codecov Report

Merging #310 (5f144a2) into master (7e93511) will decrease coverage by 0.02%.
The diff coverage is 80.00%.

@@             Coverage Diff              @@
##             master     #310      +/-   ##
============================================
- Coverage     58.92%   58.89%   -0.03%     
  Complexity     1206     1206              
============================================
  Files           131      131              
  Lines          5066     5068       +2     
  Branches        593      593              
============================================
  Hits           2985     2985              
- Misses         1806     1808       +2     
  Partials        275      275              

Impacted Files	Coverage Δ
.../main/java/com/amazonaws/xray/AWSXRayRecorder.java	63.58% <77.77%> (-0.17%)	⬇️
...ava/com/amazonaws/xray/AWSXRayRecorderBuilder.java	68.75% <100.00%> (-0.55%)	⬇️
.../java/com/amazonaws/xray/entities/SegmentImpl.java	87.34% <0.00%> (-3.80%)	⬇️
...va/com/amazonaws/xray/entities/SubsegmentImpl.java	76.05% <0.00%> (+4.22%)	⬆️

---

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 7e93511...5f144a2. Read the comment docs.

[willarmiros]

Code lgtm, though one thing I'm concerned about in general is thread safety. Having several recorders use the same emitter means this path could be executed concurrently for the same socket:

aws-xray-sdk-java/aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/emitters/UDPEmitter.java

Lines 111 to 124 in 7e93511

	private boolean sendData(byte[] data, Entity entity) {
	try {
	DatagramPacket packet = new DatagramPacket(sendBuffer, DAEMON_BUF_RECEIVE_SIZE, config.getAddressForEmitter());
	packet.setData(data);
	logger.debug("Sending UDP packet.");
	daemonSocket.send(packet);
	} catch (Exception e) {
	String segmentName = Optional.ofNullable(entity.getParent()).map(this::nameAndId).orElse("[no parent segment]");
	logger.error("Exception while sending segment over UDP for entity " + nameAndId(entity) + " on segment "
	+ segmentName, e);
	return false;
	}
	return true;
	}

Is there any possibility of interference, overload, or data corruption for all that traffic going to a socket at once? Also if I may ask, what is your use case that requires several recorders which share an emitter? Generally speaking the recorder is meant to be a singleton, with only one per application.

[hden]

We've been testing with thread safety as well, so far we have not seen any evidence of interference, overload, or data corruption.

Our use-case is exactly thread safety. We have been handling concurrent workload on a thread-pool (in Clojure). We started with a singleton, but it's quite unrealistic to keep track with segment context since context switching is everywhere. So we've been trying with the idea that we could create one recorder per job (a request, to be precise) and pass the recorder along the job context instead.

cc @popoppo What do you think?