Skip to content

Improve telemetry for the diff state model#12162

Merged
MaggieShan merged 3 commits into
masterfrom
maggs/improve-diffstate-telem
Jun 3, 2026
Merged

Improve telemetry for the diff state model#12162
MaggieShan merged 3 commits into
masterfrom
maggs/improve-diffstate-telem

Conversation

@MaggieShan
Copy link
Copy Markdown
Contributor

@MaggieShan MaggieShan commented Jun 3, 2026

Description

  • With the launch of remote support for the code review view, we want better insights into how users are interacting with the feature and what kind of errors they're running into.
    • This PR introduces a more generic DiffStateError type to help classify the different kinds of errors a user can run into when loading diffs/running git commands
      • This helps not only sanitize the logs/errors that get reported to Sentry but also makes it more explicit which category of errors are expected/retry-able/transient
  • This also introduces a backend origin so that we can better differentiate between a local and remote clients
    • Previously we were only using is_local which also returns true for a diff state model running on the remote daemon, this made it hard to parse which errors were actually from a local machine vs. remote machine

Linked Issue

APP-4634
APP-4631

  • Example: we were getting sentry errors that were reporting File invalidation error: Failed to execute git command: No such file or directory which is typically an expected user/environment error that signifies git is not installed on a user's machine

Testing

  • I have manually tested my changes locally with ./script/run

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

@cla-bot cla-bot Bot added the cla-signed label Jun 3, 2026
@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented Jun 3, 2026

@MaggieShan

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 /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

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

Overview

This PR introduces a shared DiffStateError classifier, adds backend-origin and operation dimensions to diff failure telemetry, and wires those through local and remote diff-state error paths.

Concerns

  • Raw diff-state error messages are now sent through code-review telemetry paths that still classify events as non-UGC, so repo paths, refs, command output, and possible secrets bypass the telemetry UGC redaction/routing path.
  • Client-side remote diff errors are reported to Sentry after being replayed from the daemon as plain strings, which duplicates daemon-side captures for the same failure and loses the original error chain used for actionability.
  • No approved spec context was provided; no material spec drift was assessed.

Verdict

Found: 1 critical, 1 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread app/src/code_review/diff_state/local.rs Outdated
.take()
.map(|start| start.elapsed());
let err = DiffStateError::from_message(&msg);
warp_core::report_error!(&err);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ [IMPORTANT] Reporting the daemon-provided DiffState::Error again on the client duplicates the daemon-side capture for the same load failure and wraps only the flattened string, so Unknown errors lose the original chain used for actionability; keep this branch to telemetry/state updates and reserve report_error! for client-originated failures such as empty diff data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it'd still be valuable to have the client and daemon both reporting the errors - there may be cases where the failure doesn't make it back to the client

@MaggieShan MaggieShan requested a review from kevinyang372 June 3, 2026 21:32
@MaggieShan MaggieShan merged commit 856c74b into master Jun 3, 2026
28 checks passed
@MaggieShan MaggieShan deleted the maggs/improve-diffstate-telem branch June 3, 2026 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants