EIM-878: telemetry update#905
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the telemetry system to collect structured installation metrics, improve user privacy via PII scrubbing and error hashing, and update documentation accordingly. Feedback on the changes highlights two potential panic risks in the new telemetry module: first, String::truncate in the PII scrubbing function may panic if the truncation index falls on a non-UTF-8 character boundary; second, tokio::spawn in the synchronous dispatch function can panic if invoked outside of an active Tokio runtime context.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if out.len() > 1024 { | ||
| out.truncate(1024); | ||
| out.push('…'); | ||
| } |
There was a problem hiding this comment.
The String::truncate method panics if the specified index does not lie on a UTF-8 character boundary. Since out can contain arbitrary characters (such as non-ASCII characters in error messages or redacted paths), truncating at exactly 1024 bytes can cause a panic and crash the application. To prevent this, find the nearest valid character boundary at or below 1024 bytes before truncating.
| if out.len() > 1024 { | |
| out.truncate(1024); | |
| out.push('…'); | |
| } | |
| if out.len() > 1024 { | |
| let mut new_len = 1024; | |
| while !out.is_char_boundary(new_len) { | |
| new_len -= 1; | |
| } | |
| out.truncate(new_len); | |
| out.push('…'); | |
| } |
| let client = client.clone(); | ||
| tokio::spawn(async move { | ||
| let _ = client | ||
| .post(&url) | ||
| .header("Content-Type", "application/x-json-stream") | ||
| .body(serial) | ||
| .send() | ||
| .await; | ||
| }); |
There was a problem hiding this comment.
Calling tokio::spawn outside of an active tokio runtime context will panic and crash the application. Since dispatch is called from synchronous functions (such as track_install_started and track_install_outcome), there is a risk of panicking if they are invoked during synchronous initialization, testing, or from standard non-async threads. Using tokio::runtime::Handle::try_current() ensures that the task is only spawned if a runtime is active, preventing unexpected crashes.
| let client = client.clone(); | |
| tokio::spawn(async move { | |
| let _ = client | |
| .post(&url) | |
| .header("Content-Type", "application/x-json-stream") | |
| .body(serial) | |
| .send() | |
| .await; | |
| }); | |
| let client = client.clone(); | |
| if let Ok(handle) = tokio::runtime::Handle::try_current() { | |
| handle.spawn(async move { | |
| let _ = client | |
| .post(&url) | |
| .header("Content-Type", "application/x-json-stream") | |
| .body(serial) | |
| .send() | |
| .await; | |
| }); | |
| } else { | |
| log::trace!("No tokio runtime available to spawn telemetry dispatch"); | |
| } |
There was a problem hiding this comment.
@Hahihula this may make sense only if you think this can break teh installer or installation just because hte telemetry part had an exception
alirana01
left a comment
alirana01
left a comment
There was a problem hiding this comment.
There are some concerns on the PR please see if they are legit and can be fixed.
| let kind = parse_error_kind(error_kind.as_deref())?; | ||
| let error = match (outcome, kind, error_message.as_deref()) { | ||
| (InstallOutcome::Failure, Some(k), Some(msg)) => Some(build_anyhow(k, msg)), | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
GUI failures drop error telemetry is this working because if I look into the he Vue callers (SimpleSetup, InstalationProgress, OfflineInstaller) only send error_message on failure, so kind stays None, error is None, and track_install_outcome never populates error_kind, error_message, or error_hash even though the FAQs say we collect those.
CLI already does the right thing via ErrorKind::from_message
| #[serde(rename_all = "camelCase")] | ||
| struct TelemetryItem { | ||
| name: String, | ||
| struct EventProps { |
There was a problem hiding this comment.
Shouldn't we be sending the eim version nad system info with the telemetry?
| (InstallOutcome::Failure, Some(err)) => { | ||
| let raw = format!("{:#}", err); | ||
| let scrubbed = scrub_pii(&raw); | ||
| let hash = hash_short(&raw); |
There was a problem hiding this comment.
we should be hashing the scrubbed string as mentioned in our faq just to avoid even making the hash contain the scrubbed fields to avoid any concerns from community
| let client = client.clone(); | ||
| tokio::spawn(async move { | ||
| let _ = client | ||
| .post(&url) | ||
| .header("Content-Type", "application/x-json-stream") | ||
| .body(serial) | ||
| .send() | ||
| .await; | ||
| }); |
There was a problem hiding this comment.
@Hahihula this may make sense only if you think this can break teh installer or installation just because hte telemetry part had an exception
| await invoke("track_event_command", { | ||
| event: "install_started", | ||
| mode: this.is_fix_mode ? "fix" : "wizard" | ||
| }); |
There was a problem hiding this comment.
Shouldn't we also send the version and installation IDs here as well?
2 participants
this is based on #903 branch so we track the new approach to the installation.