Skip to content

Add error-category telemetry to SshTunnelEvent - #6321

Open
anton-107 wants to merge 1 commit into
mainfrom
deco-28096-error-category-in-sshtunnelevent-7
Open

Add error-category telemetry to SshTunnelEvent#6321
anton-107 wants to merge 1 commit into
mainfrom
deco-28096-error-category-in-sshtunnelevent-7

Conversation

@anton-107

@anton-107 anton-107 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Changes

Add error_category to SshTunnelEvent (libs/telemetry/protos/ssh_tunnel.go) and set it at each failure site in the ssh connect flow (experimental/ssh/internal/client/client.go). Categories name the distinct early-return sites — IDE_COMMAND_NOT_ON_PATH, CLUSTER_ACCESS_DENIED, SERVER_START_TIMEOUT, USER_ABORTED, etc. — so no raw error text, cluster name, or path is logged.

Two details worth a reviewer's attention:

  • The telemetry defer now registers before the IDE precondition checks. It sat after them, so --ide failures on a missing code/cursor command returned early and emitted no event at all. They were not merely uncategorized, they were absent from the failure counts entirely.
  • The field is declared without omitempty, so a success sends TYPE_UNSPECIFIED explicitly rather than collapsing to a null that cannot be told apart from a CLI too old to report the field.

The failure outcome is collected in a small connectOutcome struct and Run uses a named return, so the deferred logger observes the error the caller sees. A cancelled context maps to USER_ABORTED and takes precedence over the category recorded at the failure site, since Ctrl-C surfaces as a cancellation from whichever call happens to observe it first.

Why

IDE-mode connections have a 40–55% failure rate, but telemetry only records that a connection failed, so the cause is invisible. The leading hypothesis was that CheckIDECommand rejects users whose IDE shell command is not on PATH — a permanent per-machine condition, which matches the observed stickiness (a retry after a failed first attempt succeeds only 17–21% of the time).

That hypothesis was untestable for a second reason beyond the missing field: those checks ran before the telemetry defer, so they produced no event. Adding the field alone would not have confirmed or refuted it. This also means the sub-5s failure bucket in the original analysis could not have contained the PATH failures.

Deliberately left uncategorized: the malformed --metadata paths fall through to UNKNOWN. --metadata is a hidden flag whose value the CLI generates itself in ToProxyCommand, so a parse failure is a CLI bug, not a user-environment blocker. Mapping it to SERVER_START_TIMEOUT would pollute the bucket that tracks unreachable servers.

Scope note: is_success still carries omitempty, so failures remain NULL rather than false. That is tracked separately and not touched here. Until it changes, count failures via error_category (NOT IN ('TYPE_UNSPECIFIED'), plus an IS NOT NULL guard for rows from CLIs predating this field) rather than is_success = false.

The matching backend schema change has landed, so these values are queryable once this rolls out. Every enum spelling matches the constants added here exactly: the CLI serializes the enum name as a string, so a drift would silently decode to TYPE_UNSPECIFIED rather than fail loudly.

Tests

Unit tests in client_internal_test.go cover the category mapping: success reports TYPE_UNSPECIFIED, an attributed failure keeps its category, an unattributed one falls back to UNKNOWN, a cancellation reports USER_ABORTED and wins over the site category, and a non-zero exit after the tunnel is up is not counted as a connection failure.

Verified locally: ./task test-exp-ssh (278 unit + 4 acceptance) and full ./task lint (0 issues, all three modules).

I also drove Run with PATH emptied — the exact condition of the hypothesis above, since CheckIDECommand resolves the IDE command with exec.LookPath. The emitted payload is:

{"compute_type":"DEDICATED","ide_type":"vscode","client_mode":"IDE",
 "server_start_time_ms":0,"error_category":"IDE_COMMAND_NOT_ON_PATH"}

That confirms an event is emitted at all on this path, and that every category serializes to one of the declared enum names.

Still not verified end to end against real compute — no event has been observed landing in the telemetry table. Worth doing before this is relied on for dashboards.

No changelog fragment: the feature is under experimental/ and this is internal telemetry, matching #4881 and #6058.

This PR was written by Claude Code.

@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 8297e8e

Run: 32361933933

Env 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 1 4 274 1162 8:41
💚​ aws windows 1 4 276 1160 5:01
💚​ azure linux 1 4 273 1162 6:17
💚​ azure windows 1 4 275 1160 4:35
💚​ gcp linux 1 4 274 1162 7:34
💚​ gcp windows 1 4 276 1160 3:44
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
Top 7 slowest tests (at least 2 minutes):
duration env testname
3:49 azure linux TestAccept
3:49 gcp linux TestAccept
3:44 aws linux TestAccept
3:13 gcp windows TestAccept
3:10 aws windows TestAccept
3:04 azure windows TestAccept
2:22 aws linux TestFilerWorkspaceFilesExtensionsReadDir

Add a coarse, non-PII `error_category` to `SshTunnelEvent`, set at each
failure site in the `ssh connect` flow, so we can see why connections
fail rather than only that they fail.

Also register the telemetry defer before the IDE precondition checks.
Those returned before it, so `--ide` failures on a missing `code`/`cursor`
command emitted no event at all and were absent from the failure counts.

Co-authored-by: Isaac
@anton-107
anton-107 force-pushed the deco-28096-error-category-in-sshtunnelevent-7 branch from 461993e to 8297e8e Compare August 20, 2026 11:02
@anton-107
anton-107 marked this pull request as ready for review August 20, 2026 11:56
@anton-107
anton-107 requested a review from rugpanov August 20, 2026 11:58
@anton-107
anton-107 enabled auto-merge August 20, 2026 13:06
}

func Run(ctx context.Context, client *databricks.WorkspaceClient, opts ClientOptions) error {
func Run(ctx context.Context, client *databricks.WorkspaceClient, opts ClientOptions) (retErr error) {

@renaudhartert-db renaudhartert-db Aug 20, 2026

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.

[optional] Wondering if we could make Run a thin life-cycle wrapper. The current code is a little hard to follow with outcome in different place to ultimately being used upstream.

Something like this:

  func Run(ctx context.Context, client *databricks.WorkspaceClient, opts ClientOptions) error {
        ctx, cancel := context.WithCancel(ctx)
        defer cancel()

        outcome, err := connect(ctx, client, opts)
        logSshTunnelEvent(ctx, opts, outcome, err)
        return err
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants