analytics: streamlining host vs. recorder python/rust versions #1380
Conversation
teh-cmc
commented
Feb 22, 2023
teh-cmc
commented
| // 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(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| self.register("rust_version", version.to_string()); | ||
| self.deregister("python_version"); // can't be both! |
There was a problem hiding this comment.
maybe we should call these recording_rust/python_version for clarity, and the other for viewer_rust/python_version?
There was a problem hiding this comment.
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...
* streamline host vs recording python/rust versions * can't have it all * just format * no anonymous strings * the Rerun CLI does carry a Rust version too * fmt
