diff --git a/internal/backend/kernel/cgo.go b/internal/backend/kernel/cgo.go index cd604121..3fc26289 100644 --- a/internal/backend/kernel/cgo.go +++ b/internal/backend/kernel/cgo.go @@ -235,8 +235,15 @@ func lastError(code C.KernelStatusCode) *KernelError { const ( _ = uint(statusInvalidArgument-int(C.KernelStatusCode_InvalidArgument)) | uint(int(C.KernelStatusCode_InvalidArgument)-statusInvalidArgument) _ = uint(statusUnauthenticated-int(C.KernelStatusCode_Unauthenticated)) | uint(int(C.KernelStatusCode_Unauthenticated)-statusUnauthenticated) + _ = uint(statusPermissionDenied-int(C.KernelStatusCode_PermissionDenied)) | uint(int(C.KernelStatusCode_PermissionDenied)-statusPermissionDenied) + _ = uint(statusNotFound-int(C.KernelStatusCode_NotFound)) | uint(int(C.KernelStatusCode_NotFound)-statusNotFound) + _ = uint(statusResourceExhausted-int(C.KernelStatusCode_ResourceExhausted)) | uint(int(C.KernelStatusCode_ResourceExhausted)-statusResourceExhausted) _ = uint(statusUnavailable-int(C.KernelStatusCode_Unavailable)) | uint(int(C.KernelStatusCode_Unavailable)-statusUnavailable) _ = uint(statusTimeout-int(C.KernelStatusCode_Timeout)) | uint(int(C.KernelStatusCode_Timeout)-statusTimeout) + _ = uint(statusCancelled-int(C.KernelStatusCode_Cancelled)) | uint(int(C.KernelStatusCode_Cancelled)-statusCancelled) + _ = uint(statusDataLoss-int(C.KernelStatusCode_DataLoss)) | uint(int(C.KernelStatusCode_DataLoss)-statusDataLoss) + _ = uint(statusInternal-int(C.KernelStatusCode_Internal)) | uint(int(C.KernelStatusCode_Internal)-statusInternal) + _ = uint(statusInvalidStmtHandle-int(C.KernelStatusCode_InvalidStatementHandle)) | uint(int(C.KernelStatusCode_InvalidStatementHandle)-statusInvalidStmtHandle) _ = uint(statusNetworkError-int(C.KernelStatusCode_NetworkError)) | uint(int(C.KernelStatusCode_NetworkError)-statusNetworkError) _ = uint(statusSqlError-int(C.KernelStatusCode_SqlError)) | uint(int(C.KernelStatusCode_SqlError)-statusSqlError) ) diff --git a/internal/backend/kernel/errors_classify.go b/internal/backend/kernel/errors_classify.go index c28a859d..73b7c39c 100644 --- a/internal/backend/kernel/errors_classify.go +++ b/internal/backend/kernel/errors_classify.go @@ -23,12 +23,19 @@ import ( // without the C import. cgo.go asserts each equals its C.KernelStatusCode_* value // at compile time, so drift from the header is a build error, not a latent bug. const ( - statusInvalidArgument = 1 - statusUnauthenticated = 2 - statusUnavailable = 6 - statusTimeout = 7 - statusNetworkError = 12 - statusSqlError = 13 + statusInvalidArgument = 1 + statusUnauthenticated = 2 + statusPermissionDenied = 3 + statusNotFound = 4 + statusResourceExhausted = 5 + statusUnavailable = 6 + statusTimeout = 7 + statusCancelled = 8 + statusDataLoss = 9 + statusInternal = 10 + statusInvalidStmtHandle = 11 + statusNetworkError = 12 + statusSqlError = 13 ) // KernelError is the Go-side structured error mapped from the kernel's KernelError @@ -58,6 +65,40 @@ func (e *KernelError) Error() string { return fmt.Sprintf("kernel: %s (code=%d%s)", e.Message, e.Code, q) } +// codeToCategory maps the kernel status code to a telemetry ErrorCategory. This is +// the Go analog of the Python driver's _CODE_TO_EXCEPTION and Node's +// mapKernelErrorToJsError: the kernel already hands us an authoritative code, so +// telemetry keys off it instead of re-guessing from the message text. Every kernel +// status code is mapped (Internal to the generic bucket, matching how Python/Node +// bucket it) so a *KernelError never depends on the message-substring fallback. +var codeToCategory = map[int]dbsqlerrint.ErrorCategory{ + statusInvalidArgument: dbsqlerrint.CategoryInvalidRequest, + statusUnauthenticated: dbsqlerrint.CategoryAuthError, + statusPermissionDenied: dbsqlerrint.CategoryPermissionError, + statusNotFound: dbsqlerrint.CategoryNotFound, + statusResourceExhausted: dbsqlerrint.CategoryRateLimitExceeded, + statusUnavailable: dbsqlerrint.CategoryConnectionError, + statusTimeout: dbsqlerrint.CategoryTimeout, + // Cancelled is generic (caller cancel, dropped future, or server-side cancel) + // and classifyError also runs for session-lifecycle ops, so use the neutral + // cancelled category rather than the statement-specific one. + statusCancelled: dbsqlerrint.CategoryCancelled, + statusDataLoss: dbsqlerrint.CategoryResultSet, + statusInternal: dbsqlerrint.CategoryGeneric, + // A closed/invalid statement handle is a use-after-close on the result path. + statusInvalidStmtHandle: dbsqlerrint.CategoryStatementClosed, + statusNetworkError: dbsqlerrint.CategoryConnectionError, + statusSqlError: dbsqlerrint.CategoryExecuteStatement, +} + +// Category satisfies the telemetry categorizer interface (read via +// CategoryFromError), so a kernel failure reports its code-derived category rather +// than one inferred from the message. Every kernel code is mapped; a value outside +// the enum returns "" and only then does classifyError fall back to the message. +func (e *KernelError) Category() dbsqlerrint.ErrorCategory { + return codeToCategory[e.Code] +} + // statementIDFromError returns the server query id carried on a KernelError, or "" // for a nil / non-KernelError error or one with no id. The execute-error path uses // it to set kernelOp.statementID (there is no exec handle to read it from), so the diff --git a/internal/backend/kernel/errors_classify_test.go b/internal/backend/kernel/errors_classify_test.go index 2140d3fa..32eeec09 100644 --- a/internal/backend/kernel/errors_classify_test.go +++ b/internal/backend/kernel/errors_classify_test.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "testing" + + dbsqlerrint "github.com/databricks/databricks-sql-go/internal/errors" ) // These guard the safety contract (which errors may trigger database/sql's @@ -149,6 +151,72 @@ func TestStatementIDFromError(t *testing.T) { } } +// Category maps the kernel code to a telemetry category (read via +// CategoryFromError), so a kernel failure reports its authoritative code instead of +// a message-inferred guess. An unmapped code returns "" to keep the message fallback. +func TestKernelErrorCategory(t *testing.T) { + cases := []struct { + code int + want dbsqlerrint.ErrorCategory + }{ + {statusInvalidArgument, dbsqlerrint.CategoryInvalidRequest}, + {statusUnauthenticated, dbsqlerrint.CategoryAuthError}, + {statusPermissionDenied, dbsqlerrint.CategoryPermissionError}, + {statusNotFound, dbsqlerrint.CategoryNotFound}, + {statusResourceExhausted, dbsqlerrint.CategoryRateLimitExceeded}, + {statusUnavailable, dbsqlerrint.CategoryConnectionError}, + {statusTimeout, dbsqlerrint.CategoryTimeout}, + {statusCancelled, dbsqlerrint.CategoryCancelled}, + {statusDataLoss, dbsqlerrint.CategoryResultSet}, + {statusInternal, dbsqlerrint.CategoryGeneric}, + {statusInvalidStmtHandle, dbsqlerrint.CategoryStatementClosed}, + {statusNetworkError, dbsqlerrint.CategoryConnectionError}, + {statusSqlError, dbsqlerrint.CategoryExecuteStatement}, + {9999, ""}, // outside the enum → "" so classifyError falls back to the message + } + for _, c := range cases { + if got := (&KernelError{Code: c.code}).Category(); got != c.want { + t.Errorf("code %d: Category() = %q, want %q", c.code, got, c.want) + } + } +} + +// allKernelStatusCodes is the full C enum (KernelStatusCode 1..13). It is the +// tripwire for the "every code is mapped" invariant: adding a status constant here +// makes TestKernelErrorCategoryExhaustive fail until codeToCategory maps it, so a +// new code can't silently fall through to the message classifier. +var allKernelStatusCodes = []int{ + statusInvalidArgument, statusUnauthenticated, statusPermissionDenied, + statusNotFound, statusResourceExhausted, statusUnavailable, statusTimeout, + statusCancelled, statusDataLoss, statusInternal, statusInvalidStmtHandle, + statusNetworkError, statusSqlError, +} + +// Every kernel status code resolves to a non-empty category, and the map holds +// exactly those codes — so a code added without a mapping (or a stray extra entry) +// fails here rather than silently regressing to message matching. +func TestKernelErrorCategoryExhaustive(t *testing.T) { + for _, code := range allKernelStatusCodes { + if (&KernelError{Code: code}).Category() == "" { + t.Errorf("code %d has no category — add it to codeToCategory", code) + } + } + if len(codeToCategory) != len(allKernelStatusCodes) { + t.Errorf("codeToCategory has %d entries, want %d (map and status-code list drifted)", + len(codeToCategory), len(allKernelStatusCodes)) + } +} + +// The category must survive the fmt.Errorf("%w") wrapping the execute/read paths +// apply, since CategoryFromError walks the chain to reach the *KernelError. This is +// the property the telemetry classifier depends on end to end. +func TestKernelErrorCategoryThroughWrap(t *testing.T) { + wrapped := fmt.Errorf("kernel: execute: %w", &KernelError{Code: statusSqlError, Message: "boom"}) + if got := dbsqlerrint.CategoryFromError(wrapped); got != dbsqlerrint.CategoryExecuteStatement { + t.Errorf("wrapped kernel error: CategoryFromError = %q, want %q", got, dbsqlerrint.CategoryExecuteStatement) + } +} + func contains(s, sub string) bool { return len(s) >= len(sub) && (s == sub || indexOf(s, sub) >= 0) } func indexOf(s, sub string) int { diff --git a/kernel_error_telemetry_e2e_test.go b/kernel_error_telemetry_e2e_test.go new file mode 100644 index 00000000..6c6dedee --- /dev/null +++ b/kernel_error_telemetry_e2e_test.go @@ -0,0 +1,119 @@ +//go:build cgo && databricks_kernel + +package dbsql + +import ( + "context" + "database/sql" + "encoding/json" + "io" + "net/http" + "strings" + "sync" + "testing" + + "github.com/databricks/databricks-sql-go/internal/config" + dbsqlerrint "github.com/databricks/databricks-sql-go/internal/errors" + "github.com/databricks/databricks-sql-go/telemetry" +) + +// captureTransport records the JSON body of every /telemetry-ext POST so a test can +// inspect what the driver actually put on the wire, while still hitting the real +// server for everything else. +type captureTransport struct { + base http.RoundTripper + mu sync.Mutex + logs []*telemetry.TelemetryEvent +} + +func (t *captureTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if strings.Contains(req.URL.Path, "/telemetry-ext") && req.Body != nil { + body, _ := io.ReadAll(req.Body) + req.Body = io.NopCloser(strings.NewReader(string(body))) + t.mu.Lock() + t.record(body) + t.mu.Unlock() + } + return t.base.RoundTrip(req) +} + +func (t *captureTransport) record(body []byte) { + var r telemetry.TelemetryRequest + if json.Unmarshal(body, &r) != nil { + return + } + for _, pl := range r.ProtoLogs { + var fl telemetry.TelemetryFrontendLog + if json.Unmarshal([]byte(pl), &fl) == nil && fl.Entry != nil && fl.Entry.SQLDriverLog != nil { + t.logs = append(t.logs, fl.Entry.SQLDriverLog) + } + } +} + +func (t *captureTransport) errorNames() []string { + t.mu.Lock() + defer t.mu.Unlock() + var names []string + for _, ev := range t.logs { + if ev.ErrorInfo != nil && ev.ErrorInfo.ErrorName != "" { + names = append(names, ev.ErrorInfo.ErrorName) + } + } + return names +} + +// TestKernelE2EErrorTelemetryCategory is the live proof of the change: a kernel SQL +// failure must report its code-derived category (execute_statement_failed for a +// SqlError) rather than the "error" message-fallback the classifier used before the +// KernelError.Category() mapping existed. +func TestKernelE2EErrorTelemetryCategory(t *testing.T) { + host, httpPath, token := pecoTestingCreds(t) + + cap := &captureTransport{base: http.DefaultTransport} + // Explicit telemetry opt-in + the capture transport, otherwise the same kernel + // connector a real consumer uses. + conn, err := NewConnector( + WithServerHostname(host), + WithHTTPPath(httpPath), + WithAccessToken(token), + WithUseKernel(true), + func(cfg *config.Config) { + cfg.EnableTelemetry = config.NewConfigValue(true) + cfg.TelemetryBatchSize = 1 + }, + ) + if err != nil { + t.Fatalf("NewConnector: %v", err) + } + c := conn.(*connector) + base := c.client.Transport + if base == nil { + base = http.DefaultTransport + } + cap.base = base + c.client.Transport = cap + + db := sql.OpenDB(conn) + defer db.Close() + + // A reference to a table that does not exist is a server-side SqlError: it + // carries a statement id (so the failure metric fires) and maps to + // CategoryExecuteStatement. + _, qErr := db.ExecContext(context.Background(), + "INSERT INTO does_not_exist_"+strings.Repeat("x", 8)+" VALUES (1)") + if qErr == nil { + t.Fatal("expected the bad-table statement to fail") + } + + // Flush pending telemetry synchronously. + db.Close() + + names := cap.errorNames() + want := string(dbsqlerrint.CategoryExecuteStatement) + for _, n := range names { + if n == want { + return + } + } + t.Fatalf("no error telemetry with error_name=%q; captured error names=%v", want, names) +}