Tag CloudFetch, Arrow schema, and result-fetch errors for telemetry - #417
Open
prathamesh-db wants to merge 2 commits into
Open
Tag CloudFetch, Arrow schema, and result-fetch errors for telemetry#417prathamesh-db wants to merge 2 commits into
prathamesh-db wants to merge 2 commits into
Conversation
Base automatically changed from
prathamesh/PECOBLR-3537-classify-reads-category
to
main
July 22, 2026 11:29
Third step of PECOBLR-3537. Attaches source-declared error categories at the error sites on the result-materialization paths, so telemetry reports a precise error_name instead of the generic "error" fallback: - chunk_download_error: CloudFetch batch download failures (internal/rows/arrowbased/batchloader.go). - arrow_schema_parsing_error: Arrow schema convert/serialize/read failures (internal/rows/arrowbased/arrowRows.go). - result_set_error: result-page fetch failure (internal/rows/rowscanner/resultPageIterator.go) and result-set metadata fetch failure (internal/rows/rows.go). A caller cancellation/deadline is left untagged at these sites so it still classifies as cancelled/timeout. - statement_execution_timeout: sentinel WatchTimeout in the poll loop (internal/backend/thrift/backend.go). This branch is unreachable in production today (the sole Watch call uses timeout=0), so it emits no telemetry; it is tagged and commented so it classifies correctly if a nonzero poll timeout is ever enabled. All tags use the existing WithCategory chaining, which returns the same concrete pointer, so errors.Is/As, the dbsqlerr.DBError assertion in the Arrow scan path, Error() strings, and sentinel identity are unchanged. Each live tag reaches classifyError via the row-iteration path (Next -> iterationErr -> AfterExecute), verified by the added tests. Separately, this fixes a latent bug in rows.getResultSetSchema where a GetResultSetMetadata failure wrapped the wrong (nil) variable instead of the real cause, dropping it from the error chain. This does not change the telemetry category (the tag wins regardless) but restores Cause()/Unwrap(). unsupported_operation (staging default case) is deferred to a follow-up: it is on a separate path and depends on more fragile invariants. Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>
prathamesh-db
force-pushed
the
prathamesh/PECOBLR-3537-tag-group-a
branch
from
July 22, 2026 11:59
9bca93d to
cf69486
Compare
prathamesh-db
marked this pull request as ready for review
July 22, 2026 12:06
mani-mathur-arch
added a commit
that referenced
this pull request
Jul 23, 2026
## What The kernel backend returns a structured `KernelError` carrying an authoritative status `Code`, but telemetry's `classifyError` was re-deriving the error category by substring-matching the error *message*. This adds `(*KernelError).Category()`, which maps the kernel status code to a telemetry `ErrorCategory`; `classifyError` reads it via `CategoryFromError` (added in #415) and only falls back to message matching for non-kernel errors. `cgo.go` pins the new status constants to the C enum at compile time. This mirrors how the other kernel-backed drivers derive the error identity from the kernel code — Python's `_CODE_TO_EXCEPTION` and Node's `mapKernelErrorToJsError`. Every kernel status code is mapped (including `Internal` → generic and `InvalidStatementHandle` → `statement_closed`, the same generic buckets Python/Node use), so a `*KernelError` never depends on the message-substring fallback. Telemetry-only: the error a caller's `Exec`/`Query` returns (full message, sqlstate, queryId via `ExecutionError`) is unchanged. ## Testing - `TestKernelErrorCategory` (all 13 kernel codes + a value outside the enum → "") and `TestKernelErrorCategoryThroughWrap` (category survives `%w` wrapping) — pure Go, run under `CGO_ENABLED=0`. - `kernel_error_telemetry_e2e_test.go` (tagged `cgo && databricks_kernel`): captures the outbound `/telemetry-ext` payload on a live warehouse and asserts a real `SqlError` lands as `execute_statement_failed`. - Verified live against a staging warehouse end to end: `SqlError → execute_statement_failed` and `Cancelled → cancelled` observed both on the wire and in the backing `frontend_log_sql_driver_log` table. ## Follow-up (not in this PR) This PR makes the mapping total over kernel *status codes*, so any `*KernelError` is code-classified. Errors that originate in the Go binding itself and carry no kernel code — Arrow schema/batch import failures and result-scan failures — can still reach the message fallback. Attaching a source-declared category at those sites is deferred, because most of them flow through the shared `internal/rows/arrowbased` layer that #417 / #419 (PECOBLR-3537, Thrift side) are already categorizing; doing it here would overlap those PRs and their new `arrowbased/errors.go`. Once they land, the residual is a small kernel-binding-only set (the cgo-boundary import failures + a couple of `operation.go` guards), which can be tagged in a focused follow-up with no conflict. This pull request and its description were written by Isaac. --------- Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
|
||
| ## Unreleased | ||
| - 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) | ||
| - Enrich telemetry error classification with source-declared error categories, so CloudFetch download, Arrow schema parsing, and result-fetch failures now report a specific `error_name` (`chunk_download_error`, `arrow_schema_parsing_error`, `result_set_error`) instead of the generic `error` (databricks/databricks-sql-go#414, #415) |
Collaborator
There was a problem hiding this comment.
nit : this is too low level for an external user. They don't need to know intrinsic details of the driver, just high level changes.
Collaborator
Author
There was a problem hiding this comment.
Simplified to a high-level summary
samikshya-db
approved these changes
Jul 27, 2026
samikshya-db
left a comment
samikshya-db
left a comment
Collaborator
There was a problem hiding this comment.
just 1 nit, LGTM otherwise. Please re-check using E2E before merging
Addresses review nit: the changelog line enumerated internal driver category names, which is too low-level for an external changelog. Co-authored-by: Isaac Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Attaches source-declared error categories at the error sites on the
result-materialization paths, so telemetry reports a precise
error_nameinstead of the genericerrorfallback:chunk_download_errorinternal/rows/arrowbased/batchloader.go)arrow_schema_parsing_errorinternal/rows/arrowbased/arrowRows.go)result_set_errorinternal/rows/rowscanner/resultPageIterator.go) and result-set metadata fetch (internal/rows/rows.go)statement_execution_timeoutWatchTimeoutin the poll loop (internal/backend/thrift/backend.go)A fetch aborted via the results context (e.g. on Close) is left untagged, so
it still classifies as
cancelled/timeoutrather thanresult_set_error,matching the CloudFetch path.
statement_execution_timeoutis unreachable in production today (the soleWatchcall usestimeout=0, so the branch never fires and emits notelemetry); it is tagged and commented so it classifies correctly if a nonzero
poll timeout is ever enabled.
Why it's safe
WithCategorychaining, which returns the sameconcrete pointer, so
errors.Is/errors.As, thedbsqlerr.DBErrorassertion in the Arrow scan path,
Error()strings, and sentinel identityare all unchanged.
classifyErrorvia the row-iteration path(
Next→iterationErr→AfterExecute), verified by the added tests.Also: a latent bug fix
rows.getResultSetSchemawrapped the wrong (nil) variable on aGetResultSetMetadatafailure, dropping the real cause from the error chain.Fixed to wrap the actual cause. This doesn't change the telemetry category (the
tag wins regardless), but it restores
Cause()/Unwrap().Design doc: https://docs.google.com/document/d/12ufP1eZrgFxWt6xzINfkhfrE-NnhB29zCa5PKokVfp8/edit
Jira: PECOBLR-3537