Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
22029c1
lazy uuid generation for SentryId and SpanId
lbloder Oct 7, 2024
f947f82
add changelog
lbloder Oct 7, 2024
debd140
Merge branch '8.x.x' into feat/lazy-span-id-v8
adinauer Oct 9, 2024
e6df0ad
add tests for lazy init, rework SentryId to cache string result
lbloder Oct 11, 2024
8a93fec
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder Oct 11, 2024
fe8ccb0
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder Oct 15, 2024
28b5bde
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder Oct 18, 2024
f2fba85
fix changelog
lbloder Oct 18, 2024
3e1a32c
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder Oct 21, 2024
c7ab294
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder Oct 22, 2024
f1e66b2
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder Oct 22, 2024
76b23c8
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder Oct 22, 2024
8599535
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder Oct 22, 2024
3bae1d7
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder Oct 22, 2024
edba0c7
Format code
getsentry-bot Oct 22, 2024
c18a74d
Add object comparison to SpanFrameMetricsCollector only check for tim…
lbloder Oct 22, 2024
35b17db
Merge branch 'feat/lazy-span-id-v8' of github.com:getsentry/sentry-ja…
lbloder Oct 22, 2024
a50d881
add proposed SentryUUID class
lbloder Oct 22, 2024
149d59c
[WIP] use SentryUUID instead of UUID.random().toString
lbloder Oct 22, 2024
6418a33
Format code
getsentry-bot Oct 23, 2024
5314e83
split SentryUUID internals into separate classes for easier attribution
lbloder Oct 29, 2024
58a00c4
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder Oct 29, 2024
097738b
add changelog
lbloder Oct 29, 2024
3518fce
Merge branch 'feat/lazy-span-id-v8' into feat/fast-id-generation
lbloder Oct 29, 2024
d11c3cc
fix tests
lbloder Oct 29, 2024
1ea4212
Merge branch '8.x.x' into feat/fast-id-generation
lbloder Oct 29, 2024
81a3bdb
fix SpanId.EMPTY_ID
lbloder Oct 29, 2024
5d00ae5
Merge branch '8.x.x' into feat/fast-id-generation
lbloder Nov 4, 2024
fac412e
test normalized is never called in no-arg constructor, only called on…
lbloder Nov 8, 2024
9f360d1
Merge branch '8.x.x' into feat/fast-id-generation
lbloder Nov 8, 2024
30b2602
fix test
lbloder Nov 8, 2024
05540a3
Format code
getsentry-bot Nov 8, 2024
143c26e
use new Random Generator
lbloder Nov 8, 2024
57d0a9b
Merge branch 'feat/fast-id-generation' of github.com:getsentry/sentry…
lbloder Nov 8, 2024
45c3cbe
fix Sentry Empty ID, replace dashes if 36 char uuid String is passed …
lbloder Nov 8, 2024
5c2c580
fix Tests for SpanId, add tests for SentryUUID and UUIDStringUtils
lbloder Nov 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Format code
  • Loading branch information
getsentry-bot committed Oct 23, 2024
commit 6418a334b4063e0b7c637e6fb3076e08caf54d92
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.Future;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.TimeUnit;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import java.io.OutputStream;
import java.io.RandomAccessFile;
import java.nio.charset.Charset;
import java.util.UUID;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.TestOnly;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.lang.reflect.Field;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.concurrent.TimeUnit;
Expand Down
1 change: 0 additions & 1 deletion sentry/src/main/java/io/sentry/ProfilingTraceData.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
import org.jetbrains.annotations.ApiStatus;
Expand Down
93 changes: 43 additions & 50 deletions sentry/src/main/java/io/sentry/SentryUUID.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
import java.util.UUID;

/**
* Utility class for faster id generation for SentryId, SpanId, and unique filenames uses throughout the SDK
* It uses our vendored Random class instead of SecureRandom for improved performance
* It directly creates a correctly formatted String to be used as IDs in the Sentry context.
* Utility class for faster id generation for SentryId, SpanId, and unique filenames uses throughout
* the SDK It uses our vendored Random class instead of SecureRandom for improved performance It
Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not yet :)
Sorry, wanted to add a comment here in the PR, that we need to first merge the main branch to 8.x.x and then to this one

* directly creates a correctly formatted String to be used as IDs in the Sentry context.
*
* Id generation is sped up by 4 to 10 times based on the underlying java version.
* <p>Id generation is sped up by 4 to 10 times based on the underlying java version.
*
* Based on the work of <a href="https://github.com/jchambers/">Jon Chambers</a>
* Here: https://github.com/jchambers/fast-uuid
* <p>Based on the work of <a href="https://github.com/jchambers/">Jon Chambers</a> Here:
* https://github.com/jchambers/fast-uuid
*/
public class SentryUUID {

Expand All @@ -21,7 +21,7 @@ public class SentryUUID {
private static final int SENTRY_SPAN_UUID_STRING_LENGTH = 16;

private static final char[] HEX_DIGITS =
new char[] { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' };
new char[] {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'};

private static final long[] HEX_VALUES = new long[128];

Expand Down Expand Up @@ -59,22 +59,20 @@ private SentryUUID() {
}

/**
* Parses a UUID from the given character sequence. The character sequence must represent a UUID as described in
* {@link UUID#toString()}.
* Parses a UUID from the given character sequence. The character sequence must represent a UUID
* as described in {@link UUID#toString()}.
*
* @param uuidSequence the character sequence from which to parse a UUID
*
* @return the UUID represented by the given character sequence
*
* @throws IllegalArgumentException if the given character sequence does not conform to the string representation as
* described in {@link UUID#toString()}
* @throws IllegalArgumentException if the given character sequence does not conform to the string
* representation as described in {@link UUID#toString()}
*/
public static UUID parseUUID(final CharSequence uuidSequence) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if we even need this

Copy link
Member

Choose a reason for hiding this comment

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

I presume no, as long as we use String values in the SDK.

if (uuidSequence.length() != UUID_STRING_LENGTH ||
uuidSequence.charAt(8) != '-' ||
uuidSequence.charAt(13) != '-' ||
uuidSequence.charAt(18) != '-' ||
uuidSequence.charAt(23) != '-') {
if (uuidSequence.length() != UUID_STRING_LENGTH
|| uuidSequence.charAt(8) != '-'
|| uuidSequence.charAt(13) != '-'
|| uuidSequence.charAt(18) != '-'
|| uuidSequence.charAt(23) != '-') {

throw new IllegalArgumentException("Illegal UUID string: " + uuidSequence);
}
Expand Down Expand Up @@ -128,11 +126,10 @@ public static String generateSpanId() {
}

/**
* Returns a string representation of the given UUID. The returned string is formatted as described in
* {@link UUID#toString()}.
* Returns a string representation of the given UUID. The returned string is formatted as
* described in {@link UUID#toString()}.
*
* @param uuid the UUID to represent as a string
*
* @return a string representation of the given UUID
*/
public static String toString(final UUID uuid) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This returns an acutal UUID String including -. Do we need this for our purposes?

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to replace all UUID usages that have - with simply using the String value without - as it should mostly be for our own IDs and some unique paths.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great, will remove this method then.

Expand All @@ -142,24 +139,24 @@ public static String toString(final UUID uuid) {

final char[] uuidChars = new char[UUID_STRING_LENGTH];

uuidChars[0] = HEX_DIGITS[(int) ((mostSignificantBits & 0xf000000000000000L) >>> 60)];
uuidChars[1] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0f00000000000000L) >>> 56)];
uuidChars[2] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00f0000000000000L) >>> 52)];
uuidChars[3] = HEX_DIGITS[(int) ((mostSignificantBits & 0x000f000000000000L) >>> 48)];
uuidChars[4] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0000f00000000000L) >>> 44)];
uuidChars[5] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00000f0000000000L) >>> 40)];
uuidChars[6] = HEX_DIGITS[(int) ((mostSignificantBits & 0x000000f000000000L) >>> 36)];
uuidChars[7] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0000000f00000000L) >>> 32)];
uuidChars[8] = '-';
uuidChars[9] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00000000f0000000L) >>> 28)];
uuidChars[0] = HEX_DIGITS[(int) ((mostSignificantBits & 0xf000000000000000L) >>> 60)];
uuidChars[1] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0f00000000000000L) >>> 56)];
uuidChars[2] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00f0000000000000L) >>> 52)];
uuidChars[3] = HEX_DIGITS[(int) ((mostSignificantBits & 0x000f000000000000L) >>> 48)];
uuidChars[4] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0000f00000000000L) >>> 44)];
uuidChars[5] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00000f0000000000L) >>> 40)];
uuidChars[6] = HEX_DIGITS[(int) ((mostSignificantBits & 0x000000f000000000L) >>> 36)];
uuidChars[7] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0000000f00000000L) >>> 32)];
uuidChars[8] = '-';
uuidChars[9] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00000000f0000000L) >>> 28)];
uuidChars[10] = HEX_DIGITS[(int) ((mostSignificantBits & 0x000000000f000000L) >>> 24)];
uuidChars[11] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0000000000f00000L) >>> 20)];
uuidChars[12] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00000000000f0000L) >>> 16)];
uuidChars[13] = '-';
uuidChars[14] = HEX_DIGITS[(int) ((mostSignificantBits & 0x000000000000f000L) >>> 12)];
uuidChars[15] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0000000000000f00L) >>> 8)];
uuidChars[16] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00000000000000f0L) >>> 4)];
uuidChars[17] = HEX_DIGITS[(int) (mostSignificantBits & 0x000000000000000fL)];
uuidChars[17] = HEX_DIGITS[(int) (mostSignificantBits & 0x000000000000000fL)];
uuidChars[18] = '-';
uuidChars[19] = HEX_DIGITS[(int) ((leastSignificantBits & 0xf000000000000000L) >>> 60)];
uuidChars[20] = HEX_DIGITS[(int) ((leastSignificantBits & 0x0f00000000000000L) >>> 56)];
Expand All @@ -177,7 +174,7 @@ public static String toString(final UUID uuid) {
uuidChars[32] = HEX_DIGITS[(int) ((leastSignificantBits & 0x000000000000f000L) >>> 12)];
uuidChars[33] = HEX_DIGITS[(int) ((leastSignificantBits & 0x0000000000000f00L) >>> 8)];
uuidChars[34] = HEX_DIGITS[(int) ((leastSignificantBits & 0x00000000000000f0L) >>> 4)];
uuidChars[35] = HEX_DIGITS[(int) (leastSignificantBits & 0x000000000000000fL)];
uuidChars[35] = HEX_DIGITS[(int) (leastSignificantBits & 0x000000000000000fL)];

return new String(uuidChars);
}
Expand Down Expand Up @@ -210,7 +207,7 @@ public static String toSentryIdString(long mostSignificantBits, long leastSignif
uuidChars[28] = HEX_DIGITS[(int) ((leastSignificantBits & 0x000000000000f000L) >>> 12)];
uuidChars[29] = HEX_DIGITS[(int) ((leastSignificantBits & 0x0000000000000f00L) >>> 8)];
uuidChars[30] = HEX_DIGITS[(int) ((leastSignificantBits & 0x00000000000000f0L) >>> 4)];
uuidChars[31] = HEX_DIGITS[(int) (leastSignificantBits & 0x000000000000000fL)];
uuidChars[31] = HEX_DIGITS[(int) (leastSignificantBits & 0x000000000000000fL)];

return new String(uuidChars);
}
Expand All @@ -219,7 +216,6 @@ public static String toSentrySpanIdString(final UUID uuid) {

final long mostSignificantBits = uuid.getMostSignificantBits();
return toSentrySpanIdString(mostSignificantBits);

}

public static String toSentrySpanIdString(long mostSignificantBits) {
Expand All @@ -230,7 +226,8 @@ public static String toSentrySpanIdString(long mostSignificantBits) {
return new String(uuidChars);
}

private static void fillMostSignificantBits(final char[] uuidChars, final long mostSignificantBits) {
private static void fillMostSignificantBits(
final char[] uuidChars, final long mostSignificantBits) {
uuidChars[0] = HEX_DIGITS[(int) ((mostSignificantBits & 0xf000000000000000L) >>> 60)];
uuidChars[1] = HEX_DIGITS[(int) ((mostSignificantBits & 0x0f00000000000000L) >>> 56)];
uuidChars[2] = HEX_DIGITS[(int) ((mostSignificantBits & 0x00f0000000000000L) >>> 52)];
Expand Down Expand Up @@ -265,13 +262,12 @@ public static long randomHalfLengthUUID() {
Random random = SentryUUID.Holder.numberGenerator;
byte[] randomBytes = new byte[8];
random.nextBytes(randomBytes);
randomBytes[6] &= 0x0f; /* clear version */
randomBytes[6] |= 0x40; /* set to version 4 */
randomBytes[6] &= 0x0f; /* clear version */
randomBytes[6] |= 0x40; /* set to version 4 */

long msb = 0;

for (int i=0; i<8; i++)
msb = (msb << 8) | (randomBytes[i] & 0xff);
for (int i = 0; i < 8; i++) msb = (msb << 8) | (randomBytes[i] & 0xff);

return msb;
}
Expand All @@ -280,27 +276,24 @@ public static UUID randomUUID() {
Random random = SentryUUID.Holder.numberGenerator;
byte[] randomBytes = new byte[16];
random.nextBytes(randomBytes);
randomBytes[6] &= 0x0f; /* clear version */
randomBytes[6] |= 0x40; /* set to version 4 */
randomBytes[8] &= 0x3f; /* clear variant */
randomBytes[8] |= (byte) 0x80; /* set to IETF variant */
randomBytes[6] &= 0x0f; /* clear version */
randomBytes[6] |= 0x40; /* set to version 4 */
randomBytes[8] &= 0x3f; /* clear variant */
randomBytes[8] |= (byte) 0x80; /* set to IETF variant */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is taken from java.uitl.UUID.
Do we need this part? Do we want/need to add those flags to our random ids?

Copy link
Member

Choose a reason for hiding this comment

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

Trying to clarify internally. Will update here when I get an answer.

Copy link
Member

Choose a reason for hiding this comment

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

We should keep it in. Also see https://develop.sentry.dev/sdk/data-model/event-payloads/#required-attributes

There's some validations for IDs, e.g.:

Let's verify those are compatible.

It's expecting a 16/32 char hexstring that isn't all zeroes. To avoid this even in case random generates an all 0 ID, we should keep the version / variant bits set.


long msb = 0;
long lsb = 0;

for (int i=0; i<8; i++)
msb = (msb << 8) | (randomBytes[i] & 0xff);
for (int i = 0; i < 8; i++) msb = (msb << 8) | (randomBytes[i] & 0xff);

for (int i=8; i<16; i++)
lsb = (lsb << 8) | (randomBytes[i] & 0xff);
for (int i = 8; i < 16; i++) lsb = (lsb << 8) | (randomBytes[i] & 0xff);

return new UUID(msb, lsb);
}

private static class Holder {
static final Random numberGenerator = new Random();
Copy link
Member

Choose a reason for hiding this comment

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

According to https://docs.oracle.com/javase/7/docs/api/java/util/Random.html there might be poor performance when using Random from multiple threads.

Instances of java.util.Random are threadsafe. However, the concurrent use of the same java.util.Random instance across threads may encounter contention and consequent poor performance. Consider instead using ThreadLocalRandom in multithreaded designs.

Should we vendor ThreadLocalRandom as well and use that?

Did you also do (performance) testing in a multi threaded environment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point. No, didn't do multithreaded perf testing. could try to spin up something to compare Random to ThreadLocalRandom

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this does make quite a difference. Below is a comparison for UUID generation:

Benchmark Score   Error Units Threads Comparison
uuidGenerationRandom 23789834,814 ± 117697,18 ops/s 1 100%
uuidGenerationRandomMultiThreaded 2280766,845 ± 263522,36 ops/s 10 10%
uuidGenerationThreadLocalRandomMultiThreaded 185639075,638 ± 5090316,9 ops/s 10 780%

Copy link
Member

Choose a reason for hiding this comment

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

whew, what's the baseline we're comparing to here? are all of those using the FastUUID code just with a different Random implementation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code from FastUUID is only responsible for converting the UUID to string. This was not done in this test.

I tested the SentryUUID.randomUUID() function, which is a clone of the java.util.UUID.randomUUID() function with java.util.Random as the random generator.

So, Results 1 and 2 test the above mentioned function in single-threaded vs multi-threaded scenarios.

Result 3 tests the same function with java.util.Random replaced with java.util.concurrent.ThreadLocalRandom. I replaced Random random = SentryUUID.Holder.numberGenerator; with Random random = ThreadLocalRandom.current();

Copy link
Collaborator Author

@lbloder lbloder Oct 29, 2024

Choose a reason for hiding this comment

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

Also, the benchmark code can now be found here: https://github.com/lbloder/sentry-uuid-bench


private Holder() {
}
private Holder() {}
}
}
3 changes: 1 addition & 2 deletions sentry/src/main/java/io/sentry/SpanId.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ public SpanId(final @NotNull String value) {
}

public SpanId() {
this.lazyValue =
new LazyEvaluator<>(SentryUUID::generateSpanId);
this.lazyValue = new LazyEvaluator<>(SentryUUID::generateSpanId);
}

@Override
Expand Down
1 change: 0 additions & 1 deletion sentry/src/main/java/io/sentry/cache/EnvelopeCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.WeakHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
Expand Down