From 6be38926d5e43c89c9dd137ebf68343f4f0a0b8b Mon Sep 17 00:00:00 2001 From: Prathamesh Baviskar Date: Mon, 20 Jul 2026 09:23:17 +0000 Subject: [PATCH 1/3] Read source-declared error category in telemetry classifyError 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 --- CHANGELOG.md | 1 + internal/errors/category.go | 21 +++++++++ internal/errors/category_test.go | 81 ++++++++++++++++++++++++++++++++ internal/errors/err.go | 24 ++++++++++ telemetry/errors.go | 7 +++ telemetry/errors_test.go | 64 +++++++++++++++++++++++++ 6 files changed, 198 insertions(+) create mode 100644 internal/errors/category_test.go create mode 100644 telemetry/errors_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 478ca97d..82372de0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # 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#XXXX) - Add an internal `ErrorCategory` type and category constants in `internal/errors` for telemetry error classification. No behavior change: the type is not read or attached anywhere yet (databricks/databricks-sql-go#414) ## v1.14.0 (2026-07-13) diff --git a/internal/errors/category.go b/internal/errors/category.go index e191b3f7..7ddf0b86 100644 --- a/internal/errors/category.go +++ b/internal/errors/category.go @@ -1,10 +1,31 @@ package errors +import "errors" + // ErrorCategory is a source-declared classification for a driver error, so // telemetry can read the category directly instead of inferring it from the // error message. type ErrorCategory string +type categorizer interface { + Category() ErrorCategory +} + +// CategoryFromError returns the first non-empty category in the error chain. +// 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 { + if c, ok := err.(categorizer); ok { + if cat := c.Category(); cat != "" { + return cat + } + } + err = errors.Unwrap(err) + } + return "" +} + const ( CategoryTimeout ErrorCategory = "timeout" CategoryCancelled ErrorCategory = "cancelled" diff --git a/internal/errors/category_test.go b/internal/errors/category_test.go new file mode 100644 index 00000000..66730693 --- /dev/null +++ b/internal/errors/category_test.go @@ -0,0 +1,81 @@ +package errors + +import ( + "context" + "testing" + + dbsqlerr "github.com/databricks/databricks-sql-go/errors" + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" +) + +func TestCategoryFromError(t *testing.T) { + t.Run("nil error has no category", func(t *testing.T) { + assert.Equal(t, ErrorCategory(""), CategoryFromError(nil)) + }) + + t.Run("untagged error has no category", func(t *testing.T) { + err := NewDriverError(context.TODO(), "boom", errors.New("cause")) + assert.Equal(t, ErrorCategory(""), CategoryFromError(err)) + }) + + t.Run("returns the category set at the source", func(t *testing.T) { + err := NewDriverError(context.TODO(), "boom", errors.New("cause")). + WithCategory(CategoryChunkDownload) + assert.Equal(t, CategoryChunkDownload, CategoryFromError(err)) + }) + + t.Run("walks the chain to a tagged inner error", func(t *testing.T) { + // errors.As would stop at the untagged outer wrapper; the walk must not. + inner := NewRequestError(context.TODO(), "inner", errors.New("cause")). + WithCategory(CategoryResultSet) + wrapped := errors.Wrap(inner, "outer") + assert.Equal(t, CategoryResultSet, CategoryFromError(wrapped)) + }) + + t.Run("outermost non-empty category wins", func(t *testing.T) { + inner := NewRequestError(context.TODO(), "inner", errors.New("cause")). + WithCategory(CategoryResultSet) + outer := NewDriverError(context.TODO(), "outer", inner). + WithCategory(CategoryChunkDownload) + assert.Equal(t, CategoryChunkDownload, CategoryFromError(outer)) + }) + + t.Run("empty outer category does not shadow a tagged inner", func(t *testing.T) { + inner := NewRequestError(context.TODO(), "inner", errors.New("cause")). + WithCategory(CategoryResultSet) + outer := NewExecutionError(context.TODO(), "outer", inner, nil) + assert.Equal(t, CategoryResultSet, CategoryFromError(outer)) + }) +} + +// WithCategory must not change the concrete type, so every public interface +// stays reachable. +func TestWithCategoryPreservesInterfaces(t *testing.T) { + t.Run("execution error", func(t *testing.T) { + err := NewExecutionError(context.TODO(), "exec", errors.New("cause"), nil). + WithCategory(CategoryStatementTimeout) + + var ee dbsqlerr.DBExecutionError + assert.True(t, errors.As(err, &ee)) + assert.Equal(t, CategoryStatementTimeout, CategoryFromError(err)) + + wrapped := errors.Wrap(err, "wrapped") + assert.True(t, errors.As(wrapped, &ee)) + assert.Equal(t, CategoryStatementTimeout, CategoryFromError(wrapped)) + }) + + t.Run("driver error", func(t *testing.T) { + err := NewDriverError(context.TODO(), "driver", errors.New("cause")). + WithCategory(CategoryChunkDownload) + var de dbsqlerr.DBDriverError + assert.True(t, errors.As(err, &de)) + }) + + t.Run("request error", func(t *testing.T) { + err := NewRequestError(context.TODO(), "request", errors.New("cause")). + WithCategory(CategoryResultSet) + var re dbsqlerr.DBRequestError + assert.True(t, errors.As(err, &re)) + }) +} diff --git a/internal/errors/err.go b/internal/errors/err.go index 1cba5a93..f6745f41 100644 --- a/internal/errors/err.go +++ b/internal/errors/err.go @@ -25,6 +25,7 @@ type databricksError struct { errType string isRetryable bool retryAfter time.Duration + category ErrorCategory } var _ error = (*databricksError)(nil) @@ -95,6 +96,11 @@ func (e databricksError) ConnectionId() string { return e.connectionId } +// Category returns the source-declared error category, or "" if none was set. +func (e databricksError) Category() ErrorCategory { + return e.category +} + func (e databricksError) Is(err error) bool { return err == dbsqlerr.DatabricksError } @@ -128,6 +134,12 @@ func NewDriverError(ctx context.Context, msg string, err error) *driverError { return &driverError{databricksError: dbErr} } +// WithCategory sets the category and returns the error, for chaining on a constructor. +func (e *driverError) WithCategory(c ErrorCategory) *driverError { + e.category = c + return e +} + // requestError are errors caused by invalid requests, e.g. permission denied, warehouse not found type requestError struct { databricksError @@ -149,6 +161,12 @@ func NewRequestError(ctx context.Context, msg string, err error) *requestError { return &requestError{databricksError: dbErr} } +// WithCategory sets the category and returns the error, for chaining on a constructor. +func (e *requestError) WithCategory(c ErrorCategory) *requestError { + e.category = c + return e +} + // executionError are errors occurring after the query has been submitted, e.g. invalid syntax, missing table, etc. type executionError struct { databricksError @@ -200,6 +218,12 @@ func NewExecutionErrorWithState(ctx context.Context, msg string, err error, quer return &executionError{databricksError: dbErr, queryId: queryId, sqlState: sqlState} } +// WithCategory sets the category and returns the error, for chaining on a constructor. +func (e *executionError) WithCategory(c ErrorCategory) *executionError { + e.category = c + return e +} + // wraps an error and adds trace if not already present func WrapErr(err error, msg string) error { var st stackTracer diff --git a/telemetry/errors.go b/telemetry/errors.go index 0702dd06..f47ef307 100644 --- a/telemetry/errors.go +++ b/telemetry/errors.go @@ -3,6 +3,8 @@ package telemetry import ( "errors" "strings" + + dbsqlerrint "github.com/databricks/databricks-sql-go/internal/errors" ) // isTerminalError returns true if error is terminal (non-retryable). @@ -50,6 +52,11 @@ func classifyError(err error) string { return "" } + // Prefer a category declared at the error source, else match the message. + if category := dbsqlerrint.CategoryFromError(err); category != "" { + return string(category) + } + errMsg := strings.ToLower(err.Error()) // Ordered patterns — first match wins, ensuring deterministic classification. diff --git a/telemetry/errors_test.go b/telemetry/errors_test.go new file mode 100644 index 00000000..6b5c926e --- /dev/null +++ b/telemetry/errors_test.go @@ -0,0 +1,64 @@ +package telemetry + +import ( + "context" + "testing" + + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + + dbsqlerrint "github.com/databricks/databricks-sql-go/internal/errors" +) + +// With no source category set, classifyError must return the same values it +// does today (the message-substring fallback). +func TestClassifyErrorFallback(t *testing.T) { + cases := []struct { + name string + msg string + want string + }{ + {"timeout", "operation timeout exceeded", "timeout"}, + {"cancelled", "context cancelled by caller", "cancelled"}, + {"connection", "connection refused", "connection_error"}, + {"authentication", "authentication failed", "auth_error"}, + {"unauthorized", "unauthorized", "auth_error"}, + {"forbidden", "forbidden", "permission_error"}, + {"not found", "table not found", "not_found"}, + {"syntax", "syntax error near 'select'", "syntax_error"}, + {"invalid", "invalid parameter", "invalid_request"}, + {"first match wins", "connection timeout", "timeout"}, + {"unmatched is generic", "something unexpected happened", "error"}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, classifyError(errors.New(c.msg))) + }) + } +} + +func TestClassifyErrorNil(t *testing.T) { + assert.Equal(t, "", classifyError(nil)) +} + +func TestClassifyErrorPrefersSourceCategory(t *testing.T) { + // Message would match "not found", but the source category wins. + err := dbsqlerrint.NewRequestError(context.TODO(), "table not found", errors.New("cause")). + WithCategory(dbsqlerrint.CategoryChunkDownload) + assert.Equal(t, "chunk_download_error", classifyError(err)) +} + +// The category must still be found when the tagged error is wrapped before +// reaching the telemetry hook. +func TestClassifyErrorSourceCategorySurvivesWrapping(t *testing.T) { + err := dbsqlerrint.NewExecutionError(context.TODO(), "boom", errors.New("cause"), nil). + WithCategory(dbsqlerrint.CategoryStatementTimeout) + wrapped := errors.Wrap(err, "while executing") + assert.Equal(t, "statement_execution_timeout", classifyError(wrapped)) +} + +func TestClassifyErrorUntaggedDbErrorUsesFallback(t *testing.T) { + err := dbsqlerrint.NewRequestError(context.TODO(), "syntax error", errors.New("cause")) + assert.Equal(t, "syntax_error", classifyError(err)) +} From dc481e5824c3046ddba600f57dd6ee30b0b4f83c Mon Sep 17 00:00:00 2001 From: Prathamesh Baviskar Date: Mon, 20 Jul 2026 09:53:28 +0000 Subject: [PATCH 2/3] Backfill CHANGELOG entry with PR number #415 Signed-off-by: Prathamesh Baviskar --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82372de0..75d1ce34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # 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#XXXX) +- 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) - Add an internal `ErrorCategory` type and category constants in `internal/errors` for telemetry error classification. No behavior change: the type is not read or attached anywhere yet (databricks/databricks-sql-go#414) ## v1.14.0 (2026-07-13) From 5c4bf391ca7af0a11eb120060495db84712d16a4 Mon Sep 17 00:00:00 2001 From: Prathamesh Baviskar Date: Tue, 21 Jul 2026 05:20:12 +0000 Subject: [PATCH 3/3] Harden ErrorCategory: innermost-wins resolution + pinned wire strings 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 --- CHANGELOG.md | 3 +- internal/errors/category.go | 29 +++++++++----- internal/errors/category_test.go | 65 +++++++++++++++++++++++++++++++- telemetry/errors.go | 27 ++++++------- 4 files changed, 98 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 75d1ce34..24e227ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,7 @@ # 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) -- Add an internal `ErrorCategory` type and category constants in `internal/errors` for telemetry error classification. No behavior change: the type is not read or attached anywhere yet (databricks/databricks-sql-go#414) +- Enrich telemetry error classification with source-declared error categories. No behavior change yet: the categories are defined and read by `classifyError` but not attached at any error source in this change (databricks/databricks-sql-go#414, #415) ## v1.14.0 (2026-07-13) - **Minimum Go version is now 1.25.0** (previously 1.20): the `go` directive was raised to 1.25.0 while clearing OSV-Scanner findings and updating dependencies. Consumers building with an older toolchain will need to upgrade Go (databricks/databricks-sql-go#368) diff --git a/internal/errors/category.go b/internal/errors/category.go index 7ddf0b86..05832b81 100644 --- a/internal/errors/category.go +++ b/internal/errors/category.go @@ -1,7 +1,5 @@ package errors -import "errors" - // ErrorCategory is a source-declared classification for a driver error, so // telemetry can read the category directly instead of inferring it from the // error message. @@ -11,17 +9,28 @@ type categorizer interface { Category() ErrorCategory } -// CategoryFromError returns the first non-empty category in the error chain. -// 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. +// CategoryFromError returns the innermost (deepest) non-empty category in the +// error chain, so a tag on an outer wrapper never masks the more specific one +// at the source. Handles both single-error and tree-shaped (errors.Join / +// multiple %w) Unwrap chains. func CategoryFromError(err error) ErrorCategory { - for err != nil { - if c, ok := err.(categorizer); ok { - if cat := c.Category(); cat != "" { - return cat + if err == nil { + return "" + } + switch x := err.(type) { + case interface{ Unwrap() error }: + if deeper := CategoryFromError(x.Unwrap()); deeper != "" { + return deeper + } + case interface{ Unwrap() []error }: + for _, e := range x.Unwrap() { + if deeper := CategoryFromError(e); deeper != "" { + return deeper } } - err = errors.Unwrap(err) + } + if c, ok := err.(categorizer); ok { + return c.Category() } return "" } diff --git a/internal/errors/category_test.go b/internal/errors/category_test.go index 66730693..e2e54aba 100644 --- a/internal/errors/category_test.go +++ b/internal/errors/category_test.go @@ -2,6 +2,8 @@ package errors import ( "context" + stderrors "errors" + "fmt" "testing" dbsqlerr "github.com/databricks/databricks-sql-go/errors" @@ -33,11 +35,20 @@ func TestCategoryFromError(t *testing.T) { assert.Equal(t, CategoryResultSet, CategoryFromError(wrapped)) }) - t.Run("outermost non-empty category wins", func(t *testing.T) { + t.Run("innermost (source) category wins over an outer tag", func(t *testing.T) { + // Pins innermost-wins: a future outer-wrapper tag must not mask the source. inner := NewRequestError(context.TODO(), "inner", errors.New("cause")). WithCategory(CategoryResultSet) outer := NewDriverError(context.TODO(), "outer", inner). WithCategory(CategoryChunkDownload) + assert.Equal(t, CategoryResultSet, CategoryFromError(outer)) + }) + + t.Run("a generic outer tag does not mask a specific inner one", func(t *testing.T) { + inner := NewRequestError(context.TODO(), "inner", errors.New("cause")). + WithCategory(CategoryChunkDownload) + outer := NewDriverError(context.TODO(), "outer", inner). + WithCategory(CategoryGeneric) assert.Equal(t, CategoryChunkDownload, CategoryFromError(outer)) }) @@ -47,6 +58,58 @@ func TestCategoryFromError(t *testing.T) { outer := NewExecutionError(context.TODO(), "outer", inner, nil) assert.Equal(t, CategoryResultSet, CategoryFromError(outer)) }) + + t.Run("finds the tag through a fmt.Errorf %w wrapper", func(t *testing.T) { + inner := NewRequestError(context.TODO(), "inner", errors.New("cause")). + WithCategory(CategoryResultSet) + wrapped := fmt.Errorf("outer: %w", inner) + assert.Equal(t, CategoryResultSet, CategoryFromError(wrapped)) + }) + + t.Run("finds the tag inside a tree-shaped chain", func(t *testing.T) { + inner := NewRequestError(context.TODO(), "inner", errors.New("cause")). + WithCategory(CategoryResultSet) + joined := stderrors.Join(errors.New("sibling"), inner) + assert.Equal(t, CategoryResultSet, CategoryFromError(joined)) + + multiWrap := fmt.Errorf("a: %w, b: %w", errors.New("sibling"), inner) + assert.Equal(t, CategoryResultSet, CategoryFromError(multiWrap)) + }) +} + +// TestCategoryStringValues pins every category to its exact wire string (a +// dashboard dimension) so a typo fails the build instead of shipping silently. +func TestCategoryStringValues(t *testing.T) { + cases := []struct { + category ErrorCategory + want string + }{ + {CategoryTimeout, "timeout"}, + {CategoryCancelled, "cancelled"}, + {CategoryConnectionError, "connection_error"}, + {CategoryAuthError, "auth_error"}, + {CategoryPermissionError, "permission_error"}, + {CategoryNotFound, "not_found"}, + {CategorySyntaxError, "syntax_error"}, + {CategoryInvalidRequest, "invalid_request"}, + {CategoryGeneric, "error"}, + {CategoryChunkDownload, "chunk_download_error"}, + {CategoryDecompression, "decompression_error"}, + {CategoryArrowSchemaParsing, "arrow_schema_parsing_error"}, + {CategoryResultSet, "result_set_error"}, + {CategoryUnsupportedOperation, "unsupported_operation"}, + {CategoryRateLimitExceeded, "rate_limit_exceeded"}, + {CategorySSLHandshake, "ssl_handshake_error"}, + {CategoryStatementTimeout, "statement_execution_timeout"}, + {CategoryExecuteStatement, "execute_statement_failed"}, + {CategoryStatementCancelled, "execute_statement_cancelled"}, + {CategorySessionClosed, "session_closed"}, + {CategoryStatementClosed, "statement_closed"}, + } + assert.Len(t, cases, 21, "every ErrorCategory constant must be pinned here") + for _, tc := range cases { + assert.Equal(t, tc.want, string(tc.category)) + } } // WithCategory must not change the concrete type, so every public interface diff --git a/telemetry/errors.go b/telemetry/errors.go index f47ef307..315959dc 100644 --- a/telemetry/errors.go +++ b/telemetry/errors.go @@ -59,30 +59,31 @@ func classifyError(err error) string { errMsg := strings.ToLower(err.Error()) - // Ordered patterns — first match wins, ensuring deterministic classification. + // Ordered patterns — first match wins. Emit the shared category constants + // so the wire strings live in one place. patterns := []struct { pattern string - errorType string + errorType dbsqlerrint.ErrorCategory }{ - {"timeout", "timeout"}, - {"context cancel", "cancelled"}, - {"connection", "connection_error"}, - {"authentication", "auth_error"}, - {"unauthorized", "auth_error"}, - {"forbidden", "permission_error"}, - {"not found", "not_found"}, - {"syntax", "syntax_error"}, - {"invalid", "invalid_request"}, + {"timeout", dbsqlerrint.CategoryTimeout}, + {"context cancel", dbsqlerrint.CategoryCancelled}, + {"connection", dbsqlerrint.CategoryConnectionError}, + {"authentication", dbsqlerrint.CategoryAuthError}, + {"unauthorized", dbsqlerrint.CategoryAuthError}, + {"forbidden", dbsqlerrint.CategoryPermissionError}, + {"not found", dbsqlerrint.CategoryNotFound}, + {"syntax", dbsqlerrint.CategorySyntaxError}, + {"invalid", dbsqlerrint.CategoryInvalidRequest}, } for _, p := range patterns { if strings.Contains(errMsg, p.pattern) { - return p.errorType + return string(p.errorType) } } // Default to generic error - return "error" + return string(dbsqlerrint.CategoryGeneric) } // httpError represents an HTTP error with status code.