Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
30 changes: 30 additions & 0 deletions internal/errors/category.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

CategoryTimeout ErrorCategory = "timeout"
CategoryCancelled ErrorCategory = "cancelled"
Expand Down
144 changes: 144 additions & 0 deletions internal/errors/category_test.go
Original file line number Diff line number Diff line change
@@ -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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

// 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))
})
}
24 changes: 24 additions & 0 deletions internal/errors/err.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type databricksError struct {
errType string
isRetryable bool
retryAfter time.Duration
category ErrorCategory
}

var _ error = (*databricksError)(nil)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
34 changes: 21 additions & 13 deletions telemetry/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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 != "" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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

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.
Expand Down
64 changes: 64 additions & 0 deletions telemetry/errors_test.go
Original file line number Diff line number Diff line change
@@ -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))
}
Loading