Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a configurable randomness source #218

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Add a configurable randomness source #218

merged 1 commit into from
Oct 2, 2020

Conversation

rhernandez35
Copy link
Contributor

@rhernandez35 rhernandez35 commented Sep 25, 2020 •
edited
Loading

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 local segment context and will try to use the
    random generator from its creator when possible, but the former is
    unresolvable without either adding more global shared state or exposing
    the RandomGenerator class.

This commit adds the following public APIs:

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

I unfortunately had to add two internal-only public methods to
AWSXRayRecorder for actually getting random values from its configured
random generator. Adding methods there allowed me to keep the
RandomGenerator class hidden from users.

There are some other minor changes in this PR:

  • Removed quadratic String concatenation in Entity#generateId
  • SegmentContextResolverChain no longer double-resolves contexts

Issue #, if available:
#216

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

willarmiros
willarmiros reviewed Sep 26, 2020
View reviewed changes
Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the changes! Left some thoughts and we should prob wait to hear from @anuraaga before getting this in.

aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/AWSXRayRecorderBuilder.java Outdated Show resolved Hide resolved
aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/AWSXRayRecorderBuilder.java Outdated Show resolved Hide resolved
aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/AWSXRayRecorder.java Outdated Show resolved Hide resolved
...order-sdk-core/src/main/java/com/amazonaws/xray/strategy/ThrowableSerializationStrategy.java Show resolved Hide resolved
aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/AWSXRayRecorder.java Outdated Show resolved Hide resolved
anuraaga
anuraaga reviewed Sep 29, 2020
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review - left a high level comment about the internal API. Thanks a lot for the help @rhernandez35!

aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/AWSXRayRecorder.java Outdated Show resolved Hide resolved
@rhernandez35
Copy link
Contributor Author

OK, I moved the random generators to internal and added a getter in AWSXRayRecorder. Everything else is mostly the same as in the last revision.

willarmiros
willarmiros reviewed Sep 30, 2020
View reviewed changes
Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more comments

aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/ThreadLocalStorage.java Outdated Show resolved Hide resolved
...order-sdk-core/src/main/java/com/amazonaws/xray/strategy/ThrowableSerializationStrategy.java Show resolved Hide resolved
@rhernandez35 rhernandez35 force-pushed the random branch 2 times, most recently from c1a2b50 to 66476ef Compare October 1, 2020 05:07
willarmiros
willarmiros approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will wait on @anuraaga to approve and merge. Thanks for your contribution and patience @rhernandez35!

@rhernandez35
Add a configurable randomness source
de5aa13 
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
anuraaga
anuraaga approved these changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rhernandez35!

@anuraaga anuraaga merged commit 9d1072a into aws:master Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willarmiros willarmiros willarmiros approved these changes

@anuraaga anuraaga anuraaga approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rhernandez35 @anuraaga @willarmiros