Replace calls to X-Ray daemon for rules / targets with a simple JDK-b… #145
Conversation
…ased client to allow removing SDK dependency in the future.
| private final URL getSamplingTargetsEndpoint; | ||
|
|
||
| public UnsignedXrayClient() { | ||
| this(new DaemonConfiguration().getEndpointForTCPConnection()); |
There was a problem hiding this comment.
Outside the scope of this PR, but we should add the DaemonConfiguration to the global recorder rather than constructing one here so that customers can programmatically set custom daemon addresses, e.g. with AWSXRay.setDaemonAddress()
|
|
||
| import java.time.Clock; | ||
| import java.util.concurrent.ScheduledExecutorService; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class TargetPollerTest { |
There was a problem hiding this comment.
Can we add a testPollerNotAbruptMainThread for the target poller as well, for parity?
There was a problem hiding this comment.
I did the opposite, removing the test from RulePoller. I don't think these actually test anything. I noticed this comment
// An Error should not be handled by the application. The executor will die and not abrupt main thread.
This comment doesn't make much sense to me - I'm not aware of any form of business logic that would cause the main thread to be interrupted from an executor (we're not passing the result back using e.g. Future) so I don't think it's possible for these tests to ever fail no matter what the code is. So they don't appear to test anything.
I don't know about executors dying either, even if a callback throws, the executor should still continue just fine.
Let me know if I'm missing anything.
There was a problem hiding this comment.
The test is indeed an invalid one. Executor would never abrupt the main thread.
I think the comment really should be
"The executor omits any exception except Error, which should stop the executor."
ScheduledExecutor dies if there's an unhandled exception. The below snippet will stop printing msg at the 5th execution
AtomicInteger i = new AtomicInteger();
Executors.newSingleThreadScheduledExecutor().scheduleAtFixedRate(() -> {
if(i.incrementAndGet() == 5) {
throw new IllegalStateException("STOP");
}
System.out.println("Hello World");
}, 0, 1000, TimeUnit.MILLISECONDS);
There was a problem hiding this comment.
Oops - thanks for clarifying the ExecutorService behavior, never noticed that!
Tweaked the wording of the comment. Believe the behavior should be tweaked a bit to follow how I've seen most libraries propagate fatal errors (AssertionError is also Error but definitely not fatal...) but will look at that in another PR.
anuraaga
force-pushed
the
url-connection-xray-client
branch
from
efd8447 to
a5e2d82
Compare
aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/internal/UnsignedXrayClient.java
Outdated
Show resolved
Hide resolved
|
|
||
| import java.time.Clock; | ||
| import java.util.concurrent.ScheduledExecutorService; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class TargetPollerTest { |
There was a problem hiding this comment.
The test is indeed an invalid one. Executor would never abrupt the main thread.
I think the comment really should be
"The executor omits any exception except Error, which should stop the executor."
ScheduledExecutor dies if there's an unhandled exception. The below snippet will stop printing msg at the 5th execution
AtomicInteger i = new AtomicInteger();
Executors.newSingleThreadScheduledExecutor().scheduleAtFixedRate(() -> {
if(i.incrementAndGet() == 5) {
throw new IllegalStateException("STOP");
}
System.out.println("Hello World");
}, 0, 1000, TimeUnit.MILLISECONDS);
|
|
||
| import java.time.Clock; | ||
| import java.util.concurrent.ScheduledExecutorService; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class TargetPollerTest { |
…ased client to allow removing SDK dependency in the future.
Issue #, if available:
For #92
Description of changes:
Instead of using the SDK to send X-Ray API requests for rules / targets, we just use the JDK's
HttpUrlConnection. This is fine because these requests are proxied by the daemon and don't need to be authenticated.I haven't removed the dependency because we expose the SDK classes in many public APIs. It seems that we have too many public methods that are only for internal use, let's aim to reduce this surface for a 3.0 release, where we should also remove the SDK dependency. When doing so, we can just vendor-in simple models corresponding to the four classes we use.
I have tried to clean stuff up in the classes I touched, especially missing
finalon many variables and an overuse of powermock (well, technically any use is overuse ;) ). I also picked wiremock as my testing framework since I saw the SDK use it too. Also took the liberty of adding assertj which SDK also uses since exception assertions are much nicer with it.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.