Read source-declared error category in telemetry classifyError by prathamesh-db · Pull Request #415 · databricks/databricks-sql-go

prathamesh-db · 2026-07-20T09:52:49Z

What

classifyError now returns the category attached to an error at its source,
via internal/errors.CategoryFromError , which walks the error chain. It falls
back to the existing message-substring matching only when no source category is
present. Adds the category field + Category() getter on the base error and
a WithCategory() setter on each concrete type to carry it.

Why

Follows #414, which added the ErrorCategory type. This wires it into the
classifier so errors can declare their category at the source instead of being
inferred from the message. No source sites are tagged yet, so classification
output is unchanged.

Design doc: https://docs.google.com/document/d/12ufP1eZrgFxWt6xzINfkhfrE-NnhB29zCa5PKokVfp8/edit
Jira: PECOBLR-3537

Second step of PECOBLR-3537. Adds the mechanism to carry a structured error category on the driver's errors and read it during telemetry classification: - add a category field + Category() getter to the base databricksError, inherited by driverError/requestError/executionError via embedding, and a WithCategory() setter on each concrete type for chaining on a constructor result; - add CategoryFromError, which walks the error chain and returns the first non-empty category (a single errors.As could bind an untagged outer error and shadow a tagged inner one); - classifyError now returns the source category when present, falling back to the existing message-substring matching otherwise. No source sites are tagged yet, so classification output is unchanged; tagging lands in follow-up PRs. Adds tests for the chain-walk, interface preservation, source-category precedence, and byte-for-byte fallback parity (none existed for the telemetry package before). Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>

Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>

samikshya-db

Code Review Squad — Review

Score: 82/100 — MODERATE RISK

Clean, correctly-layered step 2 (Option 1): category field on the embedded base struct, no import cycle, 9 legacy strings preserved byte-for-byte, genuinely behavior-neutral today (zero production WithCategory callers). Every inline finding below is latent — none breaks anything now; they bite when step 3 starts tagging error sources, which is the cheapest time to fix them. F2 and F3 are semantic choices baked into CategoryFromError worth deciding before step 3; F1 is the cheapest high-value fix (one table test).

Also (not a finding against this PR): telemetry/aggregator.go:164 feeds errorType back through isTerminalError, which matches spaced substrings ("not found", "syntax error"). The new categories are underscored tokens (not_found, chunk_download_error, …) — none match, so every source-tagged error would classify as non-terminal. Pre-existing, but argues for isTerminalError consuming the typed ErrorCategory in step 3.

Reviewers: security, architecture, language, test, devil's-advocate. Findings verified against head SHA dc481e5.

samikshya-db · 2026-07-20T16:44:02Z

 # Release History

 ## Unreleased
+- Read a source-declared error category in telemetry error classification: `classifyError` now returns the category attached to an error (via `internal/errors.CategoryFromError`, which walks the error chain) and falls back to the existing message-matching only when none is set. No source sites are tagged yet, so classification output is unchanged (databricks/databricks-sql-go#415)


you don't have to add a line for every code change, this is user facing - so it is best to keep this minimal. Please edit the previous changelog.

Done, consolidated the per-PR bullets into a single user-facing line covering the whole feature.

samikshya-db · 2026-07-21T00:27:13Z

+				return cat
+			}
+		}
+		err = errors.Unwrap(err)


[F2 · Medium] CategoryFromError is blind to tree-shaped chains (errors.Join / multi-%w)

The walk advances only via single-valued errors.Unwrap(err). errors.Join(...) and fmt.Errorf("...%w...%w...", a, b) implement Unwrap() []error, on which errors.Unwrap returns nil — so the loop terminates and returns "". Reproduced on this commit: a CategoryResultSet-tagged error inside errors.Join yields "" (expected result_set_error); same for multi-%w. Notably errors.As does find the tagged inner through a Join, so for tree chains this hand-rolled walk is strictly worse than the errors.As it was written to replace. Moot today (no errors.Join in the repo), but once step 3 tags sources and any layer aggregates errors, the source category is silently lost.

Fix: recurse into the interface{ Unwrap() []error } shape as well:

switch x := err.(type) { case interface{ Unwrap() []error }: for _, e := range x.Unwrap() { if cat := CategoryFromError(e); cat != "" { return cat } } return "" default: err = errors.Unwrap(err) }

Fixed, CategoryFromError now also traverses the Unwrap() []error shape (errors.Join / multi-%w), with test coverage.

samikshya-db · 2026-07-21T00:27:15Z

+// It walks the chain rather than using errors.As because an untagged error can
+// wrap a tagged one, and errors.As would stop at the untagged outer error.
+func CategoryFromError(err error) ErrorCategory {
+	for err != nil {


[F3 · Medium] "Outermost non-empty wins" contradicts "source-declared"; CategoryGeneric masks specific inner categories

CategoryFromError returns the outermost non-empty category, but the source of a failure is the innermost error. Because CategoryGeneric = "error" (category.go:38) is a real non-empty value, an outer wrapper tagged generic permanently shadows a precisely-tagged inner one. Confirmed on this commit: a CategoryGeneric wrapper around a CategoryChunkDownload inner returns "error", not chunk_download_error. Second problem: a source that legitimately tags CategoryGeneric emits the exact string ("error") that classifyError also returns when it gives up, so telemetry can't distinguish "source said generic" from "unclassifiable."

Fix: decide and document the winner. If source should win, collect the last non-empty category. If outermost is intended, treat CategoryGeneric as non-authoritative (skip it unless nothing more specific exists deeper).

Switched to innermost wins, the deepest (source) category now wins, so an outer wrapper's tag can never mask a more specific inner one. Verified behavior-neutral for all current tag sites; added a guard test.

samikshya-db · 2026-07-21T00:27:18Z

+		assert.Equal(t, CategoryChunkDownload, CategoryFromError(err))
+	})
+
+	t.Run("walks the chain to a tagged inner error", func(t *testing.T) {


[F5 · Low] No test for the idiomatic fmt.Errorf("%w") wrapping path

Every wrapping test uses pkg/errors.Wrap. Single-%w via fmt.Errorf is untested — it currently passes (shared errors.Unwrap mechanism), so this is a coverage gap, not a live bug; a future refactor to wrapper-specific traversal could regress it unnoticed.

Fix: add a fmt.Errorf("outer: %w", inner) sub-case (pairs naturally with the F2 regression cases).

Added fmt.Errorf("%w") and errors.Join sub-cases alongside the existing pkg/errors.Wrap coverage.

samikshya-db · 2026-07-21T00:30:11Z

+	return ""
+}
+
 const (


[F1 · High] 10 of 12 new category strings are never pinned to their literal value (re: the constant block below, esp. the 12 new categories)

classifyError emits categories via a bare return string(category) (telemetry/errors.go), so the only thing that can make a category emit a wrong telemetry string is a wrong constant literal in this block. The suite pins exactly two of the twelve new categories (chunk_download_error, statement_execution_timeout); the other ten — decompression_error, arrow_schema_parsing_error, result_set_error, unsupported_operation, rate_limit_exceeded, ssl_handshake_error, execute_statement_failed, execute_statement_cancelled, session_closed, statement_closed — appear in no _test.go at this commit. A typo like "decompresion_error" would compile, pass the whole suite, and silently emit an unrecognized dashboard dimension the moment a source site is tagged. The category_test.go assertions that use these constants compare CategoryFromError(...) to the constant itself, which is tautological w.r.t. the string value.

Fix: add one table-driven test pinning every constant to a hand-written literal (not string(cat)), ideally routed through classifyError(New...().WithCategory(cat)) so it also guards the cast.

Added TestCategoryStringValues pinning all 21 category constants to their literal wire strings

samikshya-db · 2026-07-21T00:30:13Z

 	}

+	// Prefer a category declared at the error source, else match the message.
+	if category := dbsqlerrint.CategoryFromError(err); category != "" {


[F4 · Low] Fallback pattern literals (below, ~L67) duplicate the ErrorCategory constant values

This new branch returns string(category) from the source-of-truth constants, but the message-substring fallback table a few lines down ({"timeout", "timeout"}, {"not found", "not_found"}, …) hardcodes string literals equal to those same constant values rather than deriving them. After this PR the same logical category is emitted by two independent paths; renaming a constant later would silently emit two different errorType values for the same error — the drift the #414 constants exist to prevent.

Fix: reference the constants in the fallback table, e.g. {"timeout", string(dbsqlerrint.CategoryTimeout)}; the import edge already exists.

Fixed, the fallback table's emitted values now reference the ErrorCategory constants (byte-identical output)

Follow-up to the #415 review (no runtime behavior change to the tagged error sites): - Pin every ErrorCategory constant to its exact wire string in a table test, so a typo in a dashboard dimension fails the build instead of silently emitting a wrong string (the prior assertions compared a category to the same constant, which could not catch a bad value). - CategoryFromError now returns the innermost (source) non-empty category instead of the outermost, so a category set on an outer wrapper can never mask the more specific category declared at the failure source. Every current tag is on an innermost source with untagged wrappers, so this is behavior-neutral today; it makes the "source-declared" contract explicit and future-proof. - CategoryFromError also traverses tree-shaped chains (errors.Join / multiple %w) via the Unwrap() []error shape, in addition to single-error Unwrap. Adds fmt.Errorf %w and errors.Join test coverage. - Reference the shared ErrorCategory constants in classifyError's message-substring fallback table instead of duplicating their string literals, keeping the wire strings in one place. Output is byte-identical. - Consolidate the CHANGELOG into a single minimal user-facing entry. Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>

prathamesh-db requested a review from samikshya-db July 20, 2026 09:52

prathamesh-db self-assigned this Jul 20, 2026

Backfill CHANGELOG entry with PR number #415

dc481e5

Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>

samikshya-db reviewed Jul 21, 2026

View reviewed changes

prathamesh-db requested a review from samikshya-db July 21, 2026 07:06

prathamesh-db mentioned this pull request Jul 21, 2026

Tag CloudFetch, Arrow schema, and result-fetch errors for telemetry #417

Draft

Uh oh!

Conversation

prathamesh-db commented Jul 20, 2026

What

Why

Uh oh!

samikshya-db left a comment

Choose a reason for hiding this comment

Code Review Squad — Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants