-
Notifications
You must be signed in to change notification settings - Fork 373
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: register SDK language and data source #1371
Conversation
We would always log `Python SDK` as the source
recording_source
for Rust
I don't think we should go with the name |
Maybe we should leave the patch version off the |
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.
This is absolutely fantastic.
I think this gives us a nice opportunity to streamline how we keep track of python & rust versions across host and recordings too (in another PR), see my inline comment.
Checklist:
- rust viewer cli
- rust spawn
- rust viewer - python connect
- rust viewer - rust connect
- rust viewer - then manually open python rrd
- rust viewer - then manually open rust rrd
- rust viewer - python rrd straight from cli
- rust viewer - rust rrd straight from cli
- rust serve
- not working, as expected
- python viewer cli
- python spawn
- python viewer - rust connect
- python viewer - python connect
- python viewer - then manually open rust rrd
- python viewer - then manually open python rrd
- python viewer - python rrd straight from cli
- python viewer - rust rrd straight from cli
- python serve
- not working, as expected
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()); |
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.
We can actually avoid this issue entirely: the python version that corresponds to the machine the user is running on (i.e. the machine that hosts the viewer) should never be included in events, it should only be part of the user profile (along with the rust version that's already there) i.e. update_metadata
.
The viewer_started
event then only keeps track of the app environment, while the open_recording
event always keeps track of the python_version
that corresponds to the machine doing the recording, never the host.
We can also put the rust version of the machine doing the recording in there, for consistency.
That way everything becomes consistent and relatively straightforward to reason about:
- The python & rust versions seen in user profiles (
update_metadata
) are those of the host machine (the one hosting the viewer). - The python & rust versions seen in events correspond to the machine doing the recording.
I think this changes are trivial to add on top of everything you just did?
If we agree on that I'll just make another PR.
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.
sounds good!
Co-authored-by: Clement Rey <[email protected]>
* Fix analytics recording_source for Rust We would always log `Python SDK` as the source * New analytics event: data source * Refactor: remove unused App::new * new_recording -> open_recording * Log `open_recording` event when opening files * Store data_source as part of the LogDb and put it in the same avent * Register what app environment we are running the viewer in * Also register web viewer * Add Python version * More explicitly handle recording source in analytics * compilation fix * Fix web build * Silence some clippy warnings on wasm builds * rerun_cli Co-authored-by: Clement Rey <[email protected]> --------- Co-authored-by: Clement Rey <[email protected]>
Closes most of #396
Contains a bug fix for what SDK the log events originate from (we would before always set
Python SDK
as the source).Adds a new field
data_source
which can be any of:file
(.rrd)sdk
(likelyshow()
)ws_client
(likelyspawn()
)tcp_server
(likelyconnect()
)Adds a
app_env
field that be any ofpython_sdk
,rust_sdk
,rust_cli
,web
(the latter won't happen until we have analytics on web).Also adds a
python_version
when applicable.Example
Starting
rerun
rust binary with CLI, then connecting to it from Python SDK:TODO
data_source
as part of theopen_recording
eventSend an event whenseparate PRserve()
is called (serving a web browser)Checklist
CHANGELOG.md
(if this is a big enough change to warrant it)