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

analytics: streamlining host vs. recorder python/rust versions #1380

Merged
merged 6 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/re_analytics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,11 @@ impl Analytics {
self.default_append_props.insert(name.into(), prop.into());
}

/// Deregister a property.
pub fn deregister_append_property(&mut self, name: &'static str) {
self.default_append_props.remove(name);
}

/// Record an event.
///
/// It will be extended with an `event_id` and, if this is an [`EventKind::Append`],
Expand Down
4 changes: 3 additions & 1 deletion crates/re_log_types/src/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,9 @@ fn test_encode_decode() {
recording_id: crate::RecordingId::random(),
is_official_example: true,
started: Time::now(),
recording_source: crate::RecordingSource::RustSdk,
recording_source: crate::RecordingSource::RustSdk(
env!("CARGO_PKG_RUST_VERSION").into(),
),
},
})];

Expand Down
6 changes: 3 additions & 3 deletions crates/re_log_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ impl std::fmt::Display for PythonVersion {
patch,
suffix,
} = self;
write!(f, "{}.{}.{}{}", major, minor, patch, suffix)
write!(f, "{major}.{minor}.{patch}{suffix}")
}
}

Expand All @@ -253,7 +253,7 @@ pub enum RecordingSource {
PythonSdk(PythonVersion),

/// The official Rerun Rust Logging SDK
RustSdk,
RustSdk(String),
teh-cmc marked this conversation as resolved.
Show resolved Hide resolved

/// Perhaps from some manual data ingestion?
Other(String),
Expand All @@ -264,7 +264,7 @@ impl std::fmt::Display for RecordingSource {
match self {
Self::Unknown => "Unknown".fmt(f),
Self::PythonSdk(version) => write!(f, "Python {version} SDK"),
Self::RustSdk => "Rust SDK".fmt(f),
Self::RustSdk(version) => write!(f, "Rust {version} SDK"),
teh-cmc marked this conversation as resolved.
Show resolved Hide resolved
Self::Other(string) => format!("{string:?}").fmt(f), // put it in quotes
}
}
Expand Down
9 changes: 6 additions & 3 deletions crates/re_sdk/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl Session {
Self {
enabled,

recording_source: RecordingSource::RustSdk,
recording_source: RecordingSource::RustSdk(env!("CARGO_PKG_RUST_VERSION").into()),

#[cfg(feature = "web")]
tokio_rt: tokio::runtime::Runtime::new().unwrap(),
Expand Down Expand Up @@ -399,8 +399,11 @@ impl Session {
RecordingSource::PythonSdk(python_version) => {
re_viewer::AppEnvironment::PythonSdk(python_version.clone())
}
RecordingSource::Unknown | RecordingSource::RustSdk | RecordingSource::Other(_) => {
re_viewer::AppEnvironment::RustSdk
RecordingSource::RustSdk(rust_version) => {
re_viewer::AppEnvironment::RustSdk(rust_version.clone())
}
RecordingSource::Unknown | RecordingSource::Other(_) => {
re_viewer::AppEnvironment::RustSdk("unknown".into())
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/re_viewer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub enum AppEnvironment {
PythonSdk(PythonVersion),

/// Created from the Rerun Rust SDK.
RustSdk,
RustSdk(String),
teh-cmc marked this conversation as resolved.
Show resolved Hide resolved

/// Running the Rust `rerun` binary from the CLI.
RerunCli,
Expand Down
50 changes: 37 additions & 13 deletions crates/re_viewer/src/viewer_analytics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ impl ViewerAnalytics {
analytics.register_append_property(name, property);
}
}

/// Deregister a property.
#[cfg(all(not(target_arch = "wasm32"), feature = "analytics"))]
fn deregister(&mut self, name: &'static str) {
if let Some(analytics) = &mut self.analytics {
analytics.deregister_append_property(name);
}
}
}

// ----------------------------------------------------------------------------
Expand All @@ -61,29 +69,40 @@ impl ViewerAnalytics {
use crate::AppEnvironment;
let app_env_str = match app_env {
AppEnvironment::PythonSdk(_) => "python_sdk",
AppEnvironment::RustSdk => "rust_sdk",
AppEnvironment::RustSdk(_) => "rust_sdk",
AppEnvironment::RerunCli => "rerun_cli",
AppEnvironment::Web => "web",
};
self.register("app_env", app_env_str.to_owned());

if let AppEnvironment::PythonSdk(version) = app_env {
self.register("python_version", version.to_string());
}

#[cfg(all(not(target_arch = "wasm32"), feature = "analytics"))]
if let Some(analytics) = &self.analytics {
let rerun_version = env!("CARGO_PKG_VERSION");
let rust_version = env!("CARGO_PKG_RUST_VERSION");
let target = re_analytics::TARGET_TRIPLET;
let git_hash = re_analytics::GIT_HASH;

let mut event = Event::update("update_metadata".into())
.with_prop("rerun_version".into(), rerun_version.to_owned())
.with_prop("rust_version".into(), rust_version.to_owned())
.with_prop("target".into(), target.to_owned())
.with_prop("git_hash".into(), git_hash.to_owned());

// If we happen to know the Python or Rust version used on the _host machine_, i.e. the
// machine running the viewer, then add it to the permanent user profile.
//
// The Python/Rust versions appearing in user profiles always apply to the host
// environment, _not_ the environment in which the data logging is taking place!
if let AppEnvironment::RustSdk(version) = &app_env {
event = event.with_prop("rust_version".into(), version.clone());
} else {
event = event.with_prop(
"rust_version".into(),
env!("CARGO_PKG_RUST_VERSION").to_owned(),
);
}
Copy link
Member

Choose a reason for hiding this comment

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

We are in the re_viewer crate here - how could the version in AppEnvironment::RustSdk ever differ from CARGO_PKG_RUST_VERSION? Seems like it is the same thing, just with a lot more steps?

Copy link
Member Author

@teh-cmc teh-cmc Feb 23, 2023

Choose a reason for hiding this comment

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

It can't differ, I just wanted the code to be perfectly symmetric on both sides to reflect that the versions that appear in AppEnvironment vs. RecordingSource are in fact two sides of the same coin basically.

I still think the symmetry in code is nice for comprehension, though I agree this if/else statement is quite fugly... The only reason it is there is because RerunCli doesn't carry its version around right now, but it should I think.

if let AppEnvironment::PythonSdk(version) = app_env {
event = event.with_prop("python_version".into(), version.to_string());
}

// Append opt-in metadata.
// In practice this is the email of Rerun employees
// who register their emails with `rerun analytics email`.
Expand Down Expand Up @@ -124,17 +143,22 @@ impl ViewerAnalytics {
let recording_source = match &rec_info.recording_source {
RecordingSource::Unknown => "unknown".to_owned(),
RecordingSource::PythonSdk(_version) => "python_sdk".to_owned(),
RecordingSource::RustSdk => "rust_sdk".to_owned(),
RecordingSource::RustSdk(_version) => "rust_sdk".to_owned(),
RecordingSource::Other(other) => other.clone(),
};

// If we happen to know the Python or Rust version used on the _recording machine_,
// then append it to all future events.
//
// The Python/Rust versions appearing in events always apply to the recording
// environment, _not_ the environment in which the viewer is running!
if let RecordingSource::RustSdk(version) = &rec_info.recording_source {
self.register("rust_version", version.to_string());
self.deregister("python_version"); // can't be both!
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should call these recording_rust/python_version for clarity, and the other for viewer_rust/python_version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hesitated about doing so but in my experience this always turns out to be a mistake that make even the simplest queries become awful to write.

E.g. you want everything with a rust_version, and now you have to do weird things; and soon it won't be 1 field that has these annoying quirks, it'll be 10, then 100 of them...

}
if let RecordingSource::PythonSdk(version) = &rec_info.recording_source {
// This will over-write what is set by `AppEnvironment`, if any.
// Usually these would be the same, but if you are using one python version to look
// at a recording from an older python version, then they will differ.
// For simplicity's sake we ignore this. We just want a rough idea of
// what python version our users use.
self.register("python_version", version.to_string());
self.deregister("rust_version"); // can't be both!
}

self.register("recording_source", recording_source);
Expand Down