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

Isolate cached events with hashed dsn subfolder #2038

Merged
merged 12 commits into from May 19, 2022
Merged

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented May 11, 2022

📜 Description

Isolate cached events with hashed dsn subfolder

💡 Motivation and Context

Closes #2026

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

dd to the migration page.

Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

Looks good to me.
The only thing is whether we should cache the profilingTraceDirPath and outboxPath in SentryOptions instead of evaluating the cache file every time.
But they should already be cached by the sdk components that use them, so i think it's fine.

@marandaneto
Copy link
Contributor Author

Looks good to me. The only thing is whether we should cache the profilingTraceDirPath and outboxPath in SentryOptions instead of evaluating the cache file every time. But they should already be cached by the sdk components that use them, so i think it's fine.

It's only being used when creating the integrations, and it does not do IO, the File class just makes sure that it has the right file separator, etc.

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

❗ No coverage uploaded for pull request base (6.x.x@cfdd10b). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             6.x.x    #2038   +/-   ##
========================================
  Coverage         ?   80.87%           
  Complexity       ?     3176           
========================================
  Files            ?      228           
  Lines            ?    11736           
  Branches         ?     1576           
========================================
  Hits             ?     9492           
  Misses           ?     1655           
  Partials         ?      589           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfdd10b...257f04c. Read the comment docs.

@marandaneto marandaneto marked this pull request as ready for review May 13, 2022 09:05
@marandaneto marandaneto requested a review from romtsn as a code owner May 13, 2022 09:05
final StringBuilder stringBuilder = new StringBuilder(no.toString(16));

// Add preceding 0s to make it 32 bit
while (stringBuilder.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.

Do we need a 32 character long string?
I don't believe we're reducing the risk of collision with this so maybe we can go without this altogether. (seems like this came from here, they likely needed a fixed size, but we don't).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, no need to be fixed size, but iOS also uses something similar https://github.com/getsentry/sentry-cocoa/blob/e254b733f7706d4c04c02093f5ad0715ced92443/Sources/Sentry/SentryDsn.m
CC_SHA1_DIGEST_LENGTH * 2 so it might be fixed size as well?

Copy link
Member

Choose a reason for hiding this comment

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

In iOS is just looks like it's allocating a string twice the size the number of bytes used in the digest creation. It's an optimization.
It's a copy paste from:https://stackoverflow.com/questions/7570377/creating-sha1-hash-from-nsstring

See:
image

Copy link
Member

Choose a reason for hiding this comment

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

Updated

@bruno-garcia
Copy link
Member

Other than prepending the 0's it LGTM

@marandaneto
Copy link
Contributor Author

Other than prepending the 0's it LGTM

@adinauer or @romtsn in case you have the time to finish up this PR and merge it, it should be just 2 min :) thanks so much.

@romtsn romtsn merged commit 3f10556 into 6.x.x May 19, 2022
@romtsn romtsn deleted the fix/isolate-cached-events branch May 19, 2022 07:40
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.

None yet

6 participants