Use rand instead of getrandom for IDs
#614
Conversation
The `getrandom` crate uses syscalls to get *true* randomness from the OS. We do not need that high quality of randomness, we do not even need cryptographically secure randomness. This switches to the `rand` crate which uses a reseeding PRNG which is still cryptographically secure though. This is much faster, and we can switch to an even simpler PRNG if it is not fast enough still.
Codecov Report
@@ Coverage Diff @@
## master #614 +/- ##
==========================================
- Coverage 73.14% 73.11% -0.03%
==========================================
Files 57 58 +1
Lines 6583 6576 -7
==========================================
- Hits 4815 4808 -7
Misses 1768 1768 |
| uuid = { version = "1.0.0", features = ["v4", "serde"] } | ||
| uuid = { version = "1.0.0", features = ["serde"] } |
There was a problem hiding this comment.
Note that, because Uuid is publicly reexported, this could be considered a breaking change because downstream crates are also calling Uuid::new_v4 on the reexported type, and are now breaking because the function is no longer available:
EmbarkStudios/sentry-contrib-rust#27
Maybe this shouldn't have been published as a non-breaking 0.31.7 release?
(On a related sidenote, I haven't noticed this in my crate hierarchy because other dependencies were still turning on the v4 fetaure)
There was a problem hiding this comment.
That is a very good point!
Maybe it does make sense to set up cargo-semver-checks or similar tooling that would ideally catch these problems.
There was a problem hiding this comment.
That might be nice; it's been on the back of my mind for a while for the crates I maintain, but have never got to use it.
For this case, are there any plans to "correct" this (adding back v4, yanking the past 2 patch releases) or shall we let it slide?
Because it is reexported, we cannot mark new_v4() as deprecated to make users aware of the new function, unfortunately.
There was a problem hiding this comment.
@Swatinem anything to follow up on here?
There was a problem hiding this comment.
Sorry this flew under the radar for such a long time.
For this case, are there any plans to "correct" this (adding back v4, yanking the past 2 patch releases) or shall we let it slide?
given that so much time has passed, "let it slide" was the answer here.
TBH, we don’t really have a good story for yanking releases, and would rather roll forward in such cases. And since then we have also bumped the minor (aka semver major) version.
I do feel the pain, as I also use reqwest::Url instead of url::Url in way too many places.
Another problem here is that the SDK has to do "semver major" bumps all the time, also because of all the integration crates. A concrete example here is also that our own team was not able to update the SDK because it bumped the tower version which forced that version update on the consumers as well.
Long story short I’m afraid I don’t have a good solution to all this :-(
The
getrandomcrate uses syscalls to get true randomness from the OS. We do not need that high quality of randomness, we do not even need cryptographically secure randomness.This switches to the
randcrate which uses a reseeding PRNG which is still cryptographically secure though.This is much faster, and we can switch to an even simpler PRNG if it is not fast enough still.