Skip to content

Commit

Permalink
Add a configurable randomness source
Browse files Browse the repository at this point in the history
I had two goals for this change:

1. Expose the configuration option without exposing any new classes or
interfaces. We should be in strict control of possible randomness
sources so we can guarantee that they actually do provide random values
and that they provide randomness efficiently. I didn't want it to be
possible for users to tank performance by inadvertently using a single
`Random` instance.
2. Make the random source configurable and scoped to each
`AWSXRayRecorder` instance. I didn't want to introduce any more global
shared state, so this unfortunately meant I wasn't able to remove the
global SecureRandom usage in two places: `TraceId#fromString` and
`DefaultThrowableSerializationStrategy#describeInContext`. The latter is
at least aware of the current entity and will try to use the
ID generator from its creator when possible, but the former is
unresolvable without adding more global shared state.

This commit adds the following public APIs:

* `AWSXRayRecorder#useFastIdGenerator` and
`AWSXRayRecorder#useSecureIdGenerator`: for configuring the behavior
of each `AWSXRayRecorder` instance.
* `AWSXRayRecorderBuilder#useFastIdGenerator` and
`AWSXRayRecorderBuilder#useSecureIdGenerator`: same as above
* `AWSXRayRecorder#getIdGenerator`: for getting the configured
ID generator
* `ThrowableSerializationStrategy#describeInContext(Entity, Throwable,
List<Subsegment>)`: a new method that allows the current entity to be
provided to `ThrowableSerializationStrategy`. This is currently only
useful for getting the ID generator from the current entity's creator,
but I can imagine there being other uses for this later.

There are also some classes under the `internal` package that are
`public` by necessity. However, no class outside of `AWSXRayRecorder`
has an API dependency on any of them and `AWSXRayRecorder` restricts
which implementations can be used, so they're effectively hidden (or
non-customizable, anyway).

There are some other minor changes in this PR:

* Removed quadratic String concatenation when generating Entity IDs
* Removed quadratic StringBuilder char insertion when generating Trace
IDs
* `SegmentContextResolverChain` no longer double-resolves contexts
  • Loading branch information
rhernandez35 committed Oct 1, 2020
1 parent 878ec87 commit de5aa13
Show file tree
Hide file tree
Showing 20 changed files with 428 additions and 95 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public static boolean sendSegment(Segment segment) {
public static boolean sendSubegment(Subsegment subsegment) {
return AWSXRay.sendSubsegment(subsegment);
}

public static boolean sendSubsegment(Subsegment subsegment) {
return globalRecorder.sendSubsegment(subsegment);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
import com.amazonaws.xray.entities.TraceID;
import com.amazonaws.xray.exceptions.SegmentNotFoundException;
import com.amazonaws.xray.exceptions.SubsegmentNotFoundException;
import com.amazonaws.xray.internal.FastIdGenerator;
import com.amazonaws.xray.internal.IdGenerator;
import com.amazonaws.xray.internal.SecureIdGenerator;
import com.amazonaws.xray.listeners.SegmentListener;
import com.amazonaws.xray.strategy.ContextMissingStrategy;
import com.amazonaws.xray.strategy.DefaultContextMissingStrategy;
Expand Down Expand Up @@ -117,6 +120,7 @@ public class AWSXRayRecorder {
private PrioritizationStrategy prioritizationStrategy;
private ThrowableSerializationStrategy throwableSerializationStrategy;
private ContextMissingStrategy contextMissingStrategy;
private IdGenerator idGenerator;

private SegmentContextResolverChain segmentContextResolverChain;

Expand All @@ -137,6 +141,7 @@ public AWSXRayRecorder() {
prioritizationStrategy = new DefaultPrioritizationStrategy();
throwableSerializationStrategy = new DefaultThrowableSerializationStrategy();
contextMissingStrategy = new DefaultContextMissingStrategy();
idGenerator = new SecureIdGenerator();

logReferences = new HashSet<>();

Expand Down Expand Up @@ -942,6 +947,39 @@ public void setOrigin(String origin) {
this.origin = origin;
}

/**
* Configures this {@code AWSXRayRecorder} to use a fast but cryptographically insecure random number
* generator for generating random IDs. This option should be preferred if your application does not
* rely on AWS X-Ray Trace IDs being generated from a cryptographically secure random number generator.
*
* @see #useSecureIdGenerator()
*/
public final void useFastIdGenerator() {
this.idGenerator = new FastIdGenerator();
}

/**
* Configures this {@code AWSXRayRecorder} to use a cryptographically secure random generator for
* generating random IDs. Unless your application in some way relies on AWS X-Ray trace IDs
* being generated from a cryptographically secure random number source, you should prefer
* to use {@linkplain #useFastIdGenerator() the fast ID generator}.
*
* @see #useFastIdGenerator()
*/
public final void useSecureIdGenerator() {
this.idGenerator = new SecureIdGenerator();
}

/**
* Gets this {@code AWSXRayRecorder} instance's ID generator. This method is intended for
* internal use only.
*
* @return the configured ID generator
*/
public final IdGenerator getIdGenerator() {
return idGenerator;
}

/**
* Checks whether the current {@code SamplingStrategy} supports forced sampling.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ public class AWSXRayRecorderBuilder {
@Nullable
private Emitter emitter;

private boolean useFastIdGenerator = false;


private AWSXRayRecorderBuilder() {
plugins = new HashSet<>();
Expand Down Expand Up @@ -220,6 +222,31 @@ public AWSXRayRecorderBuilder withDefaultPlugins() {
return this;
}

/**
* Prepares this builder to build an {@code AWSXRayRecorder} which uses a fast but cryptographically insecure
* random number generator for generating random IDs. This option should be preferred unless your application
* relies on AWS X-Ray trace IDs being generated from a cryptographically secure random number source.
*
* @see #withSecureIdGenerator() ()
*/
public AWSXRayRecorderBuilder withFastIdGenerator() {
this.useFastIdGenerator = true;
return this;
}

/**
* Prepares this builder to build an {@code AWSXRayRecorder} which uses a cryptographically secure random
* generator for generating random IDs. Unless your application relies on AWS X-Ray trace IDs
* being generated from a cryptographically secure random number source, you should prefer
* to use {@linkplain #withFastIdGenerator() the fast ID generator}.
*
* @see #withFastIdGenerator()
*/
public AWSXRayRecorderBuilder withSecureIdGenerator() {
this.useFastIdGenerator = false;
return this;
}

/**
* Constructs and returns an AWSXRayRecorder with the provided configuration.
*
Expand Down Expand Up @@ -258,6 +285,12 @@ public AWSXRayRecorder build() {
client.addAllSegmentListeners(segmentListeners);
}

if (useFastIdGenerator) {
client.useFastIdGenerator();
} else {
client.useSecureIdGenerator();
}

plugins.stream().filter(Objects::nonNull).filter(p -> p.isEnabled()).forEach(plugin -> {
logger.info("Collecting trace metadata from " + plugin.getClass().getName() + ".");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public static void clear() {
CURRENT_ENTITY.remove();
}

@Deprecated
public static SecureRandom getRandom() {
return SECURE_RANDOM;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private static FacadeSegment newFacadeSegment(AWSXRayRecorder recorder, String n
if (isInitializing(traceHeader)) {
logger.warn(LAMBDA_TRACE_HEADER_KEY + " is missing a trace ID, parent ID, or sampling decision. Subsegment "
+ name + " discarded.");
return new FacadeSegment(recorder, TraceID.create(), "", SampleDecision.NOT_SAMPLED);
return new FacadeSegment(recorder, TraceID.create(recorder), "", SampleDecision.NOT_SAMPLED);
}
return new FacadeSegment(recorder, traceHeader.getRootTraceId(), traceHeader.getParentId(), traceHeader.getSampled());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import org.checkerframework.checker.nullness.qual.Nullable;

public class SegmentContextResolverChain implements ResolverChain<SegmentContext> {

private List<SegmentContextResolver> resolvers = new ArrayList<>();
private final List<SegmentContextResolver> resolvers = new ArrayList<>();

public void addResolver(SegmentContextResolver resolver) {
resolvers.add(resolver);
Expand All @@ -31,13 +30,13 @@ public void addResolver(SegmentContextResolver resolver) {
@Override
@Nullable
public SegmentContext resolve() {
Optional<SegmentContextResolver> firstResolver = resolvers.stream()
.filter(resolver -> resolver.resolve() != null)
.findFirst();

if (firstResolver.isPresent()) {
return firstResolver.get().resolve();
for (SegmentContextResolver resolver : resolvers) {
SegmentContext ctx = resolver.resolve();
if (ctx != null) {
return ctx;
}
}

return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public DummySegment(AWSXRayRecorder creator, String name, TraceID traceId) {
}

public DummySegment(AWSXRayRecorder creator) {
this(creator, TraceID.create());
this(creator, TraceID.create(creator));
}

public DummySegment(AWSXRayRecorder creator, TraceID traceId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class DummySubsegment implements Subsegment {
private Segment parentSegment;

public DummySubsegment(AWSXRayRecorder creator) {
this(creator, TraceID.create());
this(creator, TraceID.create(creator));
}

public DummySubsegment(AWSXRayRecorder creator, TraceID traceId) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@

package com.amazonaws.xray.entities;

import com.amazonaws.xray.AWSXRay;
import com.amazonaws.xray.AWSXRayRecorder;
import com.amazonaws.xray.ThreadLocalStorage;
import com.amazonaws.xray.exceptions.AlreadyEmittedException;
import com.fasterxml.jackson.annotation.JsonIgnore;
import java.util.List;
Expand All @@ -27,12 +27,13 @@

public interface Entity extends AutoCloseable {

/**
* @deprecated Use the {@link com.amazonaws.xray.internal.IdGenerator ID generator} configured on this
* entity's creator instead
*/
@Deprecated
static String generateId() {
String id = Long.toString(ThreadLocalStorage.getRandom().nextLong() >>> 1, 16);
while (id.length() < 16) {
id = '0' + id;
}
return id;
return AWSXRay.getGlobalRecorder().getIdGenerator().newEntityId();
}

String getName();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ protected EntityImpl(AWSXRayRecorder creator, String name) {
this.annotations = new ConcurrentHashMap<>();
this.metadata = new ConcurrentHashMap<>();
this.startTime = Instant.now().toEpochMilli() / 1000.0d;
this.id = Entity.generateId();
this.id = creator.getIdGenerator().newEntityId();
this.inProgress = true;
this.referenceCount = new LongAdder();
this.totalSize = new LongAdder();
Expand Down Expand Up @@ -445,7 +445,8 @@ public void addException(Throwable exception) {
setFault(true);
getSubsegmentsLock().lock();
try {
cause.addExceptions(creator.getThrowableSerializationStrategy().describeInContext(exception, subsegments));
cause.addExceptions(creator.getThrowableSerializationStrategy()
.describeInContext(this, exception, subsegments));
} finally {
getSubsegmentsLock().unlock();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,17 @@ private SegmentImpl() {
// it makes the code to hard to reason about e.g., nullness.
@SuppressWarnings("nullness")
public SegmentImpl(AWSXRayRecorder creator, String name) {
this(creator, name, TraceID.create());
this(creator, name, TraceID.create(creator));
}

// TODO(anuraaga): Refactor the entity relationship. There isn't a great reason to use a type hierarchy for data classes and
// it makes the code to hard to reason about e.g., nullness.
@SuppressWarnings("nullness")
public SegmentImpl(AWSXRayRecorder creator, String name, TraceID traceId) {
super(creator, name);
if (traceId == null) {
traceId = TraceID.create(creator);
}
setTraceId(traceId);

this.service = new ConcurrentHashMap<>();
Expand Down
Loading

0 comments on commit de5aa13

Please sign in to comment.