Update telemetry for code review over remote sessions#11484
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
MaggieShan
changed the title
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds an is_local telemetry dimension across code review events, moves editor-scoped comment/revert telemetry emission up to the code review view so repo locality is available, and adds a diff-load completion event with aggregate diff metadata.
Concerns
- No blocking correctness concerns found in the changed lines.
- Security pass found no issues; the new telemetry fields are limited to local/remote classification and aggregate counts/status values.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| } | ||
| RevertDiffHunk { line_range } => { | ||
| if FeatureFlag::RevertDiffHunk.is_enabled() { | ||
| send_telemetry_from_ctx!(CodeReviewTelemetryEvent::RevertHunkClicked, ctx); |
There was a problem hiding this comment.
|
|
||
| impl CodeEditorModel { | ||
| pub fn open_comment_line(&mut self, line: &EditorLineLocation, ctx: &mut ModelContext<Self>) { | ||
| // Telemetry: comment editor opened for a new inline review comment. |
There was a problem hiding this comment.
Why are we refactoring the CommentEditorOpened event?
There was a problem hiding this comment.
We're moving it so it's instead emitted from the code review view so we have access to the is_local field
| @@ -215,6 +216,14 @@ impl RemoteDiffStateModel { | |||
| Ok((metadata, state, diffs)) => self.apply_snapshot(metadata, state, diffs, ctx), | |||
| Err(error) => { | |||
| log::warn!("RemoteDiffStateModel: invalid diff state snapshot: {error}"); | |||
There was a problem hiding this comment.
Actually instead of logging telemetry, would it make more sense to make this warn statement log::error so we could track these in sentry?
One thing we should do a pass on is the message here doesn't contain PII or IDs (e.g. Session ID)
There was a problem hiding this comment.
That's a good point - can confirm that the error message themselves don't include any PII and mainly come from the try_from fns that send static strings. Updated to send error logs with safe_error! instead of telemetry events
| /// Timestamp captured when a diff load is initiated. Cleared when the | ||
| /// corresponding `DiffLoadCompleted` telemetry event is emitted, so the | ||
| /// elapsed time between request and completion can be reported. | ||
| diff_load_start_time: Option<Instant>, |
There was a problem hiding this comment.
Instead of putting this on the overall CodeReviewView struct, should it belong to InternalRemoteDiffState / InternalDiffState in diff state model? Then we could pop and log the load time once we transition from loading -> loaded / error
There was a problem hiding this comment.
Good point - will update!
MaggieShan
force-pushed
the
maggs/remote-server-code-review-telem
branch
from
3a8f222 to
d2120d8
Compare
2 participants
Description
Testing
Verified by running locally with_sandbox_telemetry
Agent mode