-
-
Notifications
You must be signed in to change notification settings - Fork 435
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.
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
Lazy uuid generation for SentryId and SpanId #3770
Merged
Merged
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
22029c1
lazy uuid generation for SentryId and SpanId
lbloder f947f82
add changelog
lbloder debd140
Merge branch '8.x.x' into feat/lazy-span-id-v8
adinauer e6df0ad
add tests for lazy init, rework SentryId to cache string result
lbloder 8a93fec
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder fe8ccb0
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder 28b5bde
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder f2fba85
fix changelog
lbloder 3e1a32c
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder c7ab294
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder f1e66b2
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder 76b23c8
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder 8599535
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder 3bae1d7
Update sentry/src/main/java/io/sentry/protocol/SentryId.java
lbloder edba0c7
Format code
getsentry-bot c18a74d
Add object comparison to SpanFrameMetricsCollector only check for tim…
lbloder 35b17db
Merge branch 'feat/lazy-span-id-v8' of github.com:getsentry/sentry-ja…
lbloder 58a00c4
Merge branch '8.x.x' into feat/lazy-span-id-v8
lbloder File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adinauer -> Please see comment from @kozaxinan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should measure impact of this PR. I'm afraid this doesn't have the desired effect due to
toString()
,hashCode
andequals
being called early. Caching the value fortoString()
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kozaxinan Good point, I'll rework
SentryId
to cache the resultingString
instead of theUUID
object.As mentioned by @adinauer, measuring the performance impact of this change has yet to be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice this will allow more improvement. Will this be ported to version 7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.