Lazy uuid generation for SentryId and SpanId #3770
Conversation
|
lbloder
requested review from
adinauer,
romtsn,
stefanosiano and
markushi
as code owners
|
Hey @lbloder, I was wondering on how did you test this part? It's not included in the PR. |
|
@Angelodaniel do you expect problems somewhere? If so, what areas? |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2601edf | 472.73 ms | 518.83 ms | 46.10 ms |
| 234085b | 560.06 ms | 587.22 ms | 27.16 ms |
| 081906a | 544.44 ms | 604.34 ms | 59.90 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2601edf | 1.70 MiB | 2.35 MiB | 669.02 KiB |
| 234085b | 1.70 MiB | 2.36 MiB | 670.52 KiB |
| 081906a | 1.70 MiB | 2.35 MiB | 665.19 KiB |
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return StringUtils.normalizeUUID(uuid.toString()).replace("-", ""); | ||
| return StringUtils.normalizeUUID(lazyValue.getValue().toString()).replace("-", ""); |
There was a problem hiding this comment.
This toString is called from multiple places. since uuid wont be changes. it is good to cache that. but also this toString will do unnecessary calculations.
Please consider holding toString value instead of uuid similar to SpanId or also cache the toString result.
There was a problem hiding this comment.
It is more related to #3772
But if toString is called right after creating SentryId, it wont help much of delaying this UUID cost.
There was a problem hiding this comment.
@adinauer -> Please see comment from @kozaxinan
There was a problem hiding this comment.
Yeah, we should measure impact of this PR. I'm afraid this doesn't have the desired effect due to toString(), hashCode and equals being called early. Caching the value for toString() might have a bigger impact, thanks for the suggestion.
Changing how we use IDs (i.e. only generate them when sending) will be a bigger change and I'm not entirely certain it's possible. Offering a configurable ID generation mechanism might be the better approach. We'll discuss internally and update once we know more.
There was a problem hiding this comment.
@kozaxinan Good point, I'll rework SentryId to cache the resulting String instead of the UUID object.
As mentioned by @adinauer, measuring the performance impact of this change has yet to be done.
There was a problem hiding this comment.
Nice this will allow more improvement. Will this be ported to version 7?
There was a problem hiding this comment.
@kozaxinan we're planning to explore multiple options to improve the SDK. We can check which of those make sense to port to v7 when we know which ones we'll keep.
# Conflicts: # CHANGELOG.md
📜 Description
Use LazyEvaluator to generate UUID on demand in
💡 Motivation and Context
Fixes #3325
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps