-
Notifications
You must be signed in to change notification settings - Fork 63
Read source-declared error category in telemetry classifyError #415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [F5 · Low] No test for the idiomatic Every wrapping test uses Fix: add a
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 != "" { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [F4 · Low] Fallback pattern literals (below, ~L67) duplicate the This new branch returns Fix: reference the constants in the fallback table, e.g.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
|
||
| 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)) | ||
| } |
There was a problem hiding this comment.
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)
classifyErroremits categories via a barereturn 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.goat 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. Thecategory_test.goassertions that use these constants compareCategoryFromError(...)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 throughclassifyError(New...().WithCategory(cat))so it also guards the cast.There was a problem hiding this comment.
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