-
-
Notifications
You must be signed in to change notification settings - Fork 440
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
Use SecureRandom instead of Random for Metrics #3495
Conversation
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.
LGTM 🚀
Thank you for the quick PR.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d6d2b2e | 463.14 ms | 545.56 ms | 82.42 ms |
a3c77bc | 375.80 ms | 445.85 ms | 70.06 ms |
c7e2fbc | 377.85 ms | 426.35 ms | 48.50 ms |
f5e1b97 | 362.53 ms | 429.31 ms | 66.78 ms |
7eccfdb | 389.94 ms | 461.29 ms | 71.35 ms |
f1fdb9f | 404.21 ms | 496.87 ms | 92.66 ms |
28c9a83 | 346.14 ms | 377.76 ms | 31.62 ms |
baaf637 | 375.71 ms | 441.58 ms | 65.87 ms |
283d83e | 348.44 ms | 392.06 ms | 43.62 ms |
61981dc | 388.65 ms | 454.96 ms | 66.31 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d6d2b2e | 1.72 MiB | 2.27 MiB | 555.05 KiB |
a3c77bc | 1.72 MiB | 2.29 MiB | 577.53 KiB |
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
f5e1b97 | 1.70 MiB | 2.28 MiB | 592.00 KiB |
7eccfdb | 1.72 MiB | 2.27 MiB | 556.93 KiB |
f1fdb9f | 1.70 MiB | 2.28 MiB | 592.08 KiB |
28c9a83 | 1.70 MiB | 2.28 MiB | 592.00 KiB |
baaf637 | 1.72 MiB | 2.27 MiB | 558.42 KiB |
283d83e | 1.72 MiB | 2.29 MiB | 577.69 KiB |
61981dc | 1.70 MiB | 2.28 MiB | 592.12 KiB |
@markushi is this on a hot path / do we know if/how much |
It's only computed once during class init and we access SecureRandom here and here already during app start. I also tried to run it on a test device, takes <1ms to execute. final long totalDurationNanos = 0;
final int n = 1000;
for (int i = 0; i < n; i++) {
final long start = System.nanoTime();
final double value = new SecureRandom().nextFloat();
totalDurationNanos += System.nanoTime() - start;
}
Log.d("Test", "took " + TimeUnit.NANOSECONDS.toMillis(totalDurationNanos) + "ms to generate " + n + " items.");
// took 129ms to generate 1000 items. |
📜 Description
In order to not get flagged by security scanners we should use
SecureRandom
in favor ofRandom
. Whilst the use-case (random flush shift) doesn't really require a strong random, I guess it's maintenance-friendlier to useSecureRandom
.💡 Motivation and Context
Fixes getsentry/sentry-react-native#3897
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps