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.

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

SessionID as string #3834

Merged
merged 51 commits into from
Nov 11, 2024
Merged

SessionID as string #3834

merged 51 commits into from
Nov 11, 2024

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented Oct 29, 2024

📜 Description

Follow-up to #3818

💡 Motivation and Context

Replace usage of UUID in Session with String and generate ID using SentryUUID class.

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

lbloder and others added 25 commits October 7, 2024 10:52
…e difference of .equals if objects are not the same
@lbloder lbloder changed the base branch from main to feat/fast-id-generation October 29, 2024 14:29
Copy link
Contributor

github-actions bot commented Oct 29, 2024

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- SessionID as string ([#3834](https://github.com/getsentry/sentry-java/pull/3834))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 22e215e

@lbloder lbloder marked this pull request as ready for review November 4, 2024 11:08
} catch (IllegalArgumentException e) {
logger.log(SentryLevel.ERROR, "%s sid is not valid.", sidString);
String sid = reader.nextStringOrNull();
if (sid != null && sid.length() == 32) {
Copy link
Member

Choose a reason for hiding this comment

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

should we still keep the log call in the else branch?

@@ -79,7 +78,7 @@ public Session(
final @Nullable Date timestamp,
final int errorCount,
final @Nullable String distinctId,
final @Nullable UUID sessionId,
final @Nullable String sessionId,
Copy link
Member

Choose a reason for hiding this comment

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

do we have to apply some validation still that this is compliant with the uuid format?

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

couple of comments to address, but lgtm otherwise

Base automatically changed from feat/fast-id-generation to 8.x.x November 11, 2024 04:57
@adinauer adinauer merged commit ac12613 into 8.x.x Nov 11, 2024
21 of 23 checks passed
@adinauer adinauer deleted the feat/session-id-as-string branch November 11, 2024 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants