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.

Sign up for GitHub

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

Jump to bottom

Use rand instead of getrandom for IDs #614

Merged
merged 2 commits into from Sep 14, 2023
Merged

Use rand instead of getrandom for IDs #614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7785fd0
Use `rand` instead of `getrandom` for IDs
Swatinem Sep 14, 2023
abc0ca6
use the random uuid in more places
Swatinem Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions sentry-core/src/api.rs
View file
Open in desktop
Expand Up @@ -17,9 +17,9 @@ use crate::{Hub, Integration, IntoBreadcrumbs, Scope};
///
/// ```
/// use sentry::protocol::{Event, Level};
/// use sentry::types::Uuid;
/// use sentry::types::{Uuid, random_uuid};
///
/// let uuid = Uuid::new_v4();
/// let uuid = random_uuid();
/// let event = Event {
/// event_id: uuid,
/// message: Some("Hello World!".into()),
Expand Down
3 changes: 2 additions & 1 deletion sentry-core/src/client.rs
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ use std::time::Duration;

use rand::random;
use sentry_types::protocol::v7::SessionUpdate;
use sentry_types::random_uuid;

use crate::constants::SDK_INFO;
use crate::protocol::{ClientSdkInfo, Event};
Expand Down Expand Up @@ -161,7 +162,7 @@ impl Client {
// event_id and sdk_info are set before the processors run so that the
// processors can poke around in that data.
if event.event_id.is_nil() {
event.event_id = Uuid::new_v4();
event.event_id = random_uuid();
}

if event.sdk.is_none() {
Expand Down
2 changes: 1 addition & 1 deletion sentry-core/src/performance.rs
View file
Open in desktop
Expand Up @@ -406,7 +406,7 @@ impl Transaction {
Some(client) => (
client.is_transaction_sampled(&ctx),
Some(protocol::Transaction {
name: Some(ctx.name.clone()),
name: Some(ctx.name),
..Default::default()
}),
),
Expand Down
4 changes: 2 additions & 2 deletions sentry-core/src/session.rs
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ use crate::protocol::{
SessionStatus, SessionUpdate,
};
use crate::scope::StackLayer;
use crate::types::Uuid;
use crate::types::random_uuid;
use crate::{Client, Envelope};

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -50,7 +50,7 @@ impl Session {
Some(Self {
client: client.clone(),
session_update: SessionUpdate {
session_id: Uuid::new_v4(),
session_id: random_uuid(),
distinct_id,
sequence: None,
timestamp: None,
Expand Down
4 changes: 2 additions & 2 deletions sentry-types/Cargo.toml
View file
Open in desktop
Expand Up @@ -22,11 +22,11 @@ protocol = []

[dependencies]
debugid = { version = "0.8.0", features = ["serde"] }
getrandom = "0.2.3"
hex = "0.4.3"
rand = "0.8.5"
serde = { version = "1.0.104", features = ["derive"] }
serde_json = "1.0.46"
thiserror = "1.0.15"
time = { version = "0.3.5", features = ["formatting", "parsing"] }
url = { version = "2.1.1", features = ["serde"] }
uuid = { version = "1.0.0", features = ["v4", "serde"] }
uuid = { version = "1.0.0", features = ["serde"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Swatinem anything to follow up on here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :-(

5 changes: 5 additions & 0 deletions sentry-types/src/lib.rs
View file
Open in desktop
Expand Up @@ -52,3 +52,8 @@ pub use crate::project_id::*;
// Re-export external types and traits for convenience
pub use debugid::*;
pub use uuid::{Uuid, Variant as UuidVariant, Version as UuidVersion};

/// Generates a random [Uuid] using a thread local RNG.
pub fn random_uuid() -> Uuid {
uuid::Builder::from_random_bytes(rand::random()).into_uuid()
}
2 changes: 1 addition & 1 deletion sentry-types/src/protocol/session.rs
View file
Open in desktop
Expand Up @@ -92,7 +92,7 @@ fn is_false(val: &bool) -> bool {
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct SessionUpdate<'a> {
/// The session identifier.
#[serde(rename = "sid", default = "Uuid::new_v4")]
#[serde(rename = "sid", default = "crate::random_uuid")]
pub session_id: Uuid,

/// The distinct identifier. Should be device or user ID.
Expand Down
16 changes: 3 additions & 13 deletions sentry-types/src/protocol/v7.rs
View file
Open in desktop
Expand Up @@ -1338,12 +1338,7 @@ pub struct SpanId([u8; 8]);

impl Default for SpanId {
fn default() -> Self {
let mut buf = [0; 8];

getrandom::getrandom(&mut buf)
.unwrap_or_else(|err| panic!("could not retrieve random bytes for SpanId: {err}"));

Self(buf)
Self(rand::random())
}
}

Expand Down Expand Up @@ -1384,12 +1379,7 @@ pub struct TraceId([u8; 16]);

impl Default for TraceId {
fn default() -> Self {
let mut buf = [0; 16];

getrandom::getrandom(&mut buf)
.unwrap_or_else(|err| panic!("could not retrieve random bytes for TraceId: {err}"));

Self(buf)
Self(rand::random())
}
}

Expand Down Expand Up @@ -1520,7 +1510,7 @@ mod event {
use super::*;

pub fn default_id() -> Uuid {
Uuid::new_v4()
crate::random_uuid()
}

pub fn serialize_id<S: Serializer>(uuid: &Uuid, serializer: S) -> Result<S::Ok, S::Error> {
Expand Down