diff --git a/CHANGELOG.md b/CHANGELOG.md index 478ca97d..24e227ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # Release History ## Unreleased -- 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 e191b3f7..05832b81 100644 --- a/internal/errors/category.go +++ b/internal/errors/category.go @@ -5,6 +5,36 @@ package errors // error message. type ErrorCategory string +type categorizer interface { + Category() ErrorCategory +} + +// 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 { + 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 + } + } + } + if c, ok := err.(categorizer); ok { + return c.Category() + } + 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..e2e54aba --- /dev/null +++ b/internal/errors/category_test.go @@ -0,0 +1,144 @@ +package errors + +import ( + "context" + stderrors "errors" + "fmt" + "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("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)) + }) + + 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)) + }) + + 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 +// 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..315959dc 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,32 +52,38 @@ 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. + // 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. 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)) +}