Tag staging, decompression, and cancellation errors for telemetry#419
Draft
prathamesh-db wants to merge 2 commits into
Draft
Tag staging, decompression, and cancellation errors for telemetry#419prathamesh-db wants to merge 2 commits into
prathamesh-db wants to merge 2 commits into
Conversation
Tags the staging default case in execStagingOperation (connection.go), which fires when the server returns a staging operation other than PUT/GET/REMOVE, with the unsupported_operation category. Previously this fell through classifyError's message matcher to the generic "error"; it now reports "unsupported_operation". The tagged *driverError reaches classifyError via the statementID-gated telemetry defer in ExecContext: reaching this case requires a staging operation (op.IsStaging), which requires a non-nil operation handle and so a non-empty StatementID. The op.ExecutionError rewrap only affects the database/sql return value, not the raw error telemetry reads. No cancellation guard is needed here: the case wraps a nil cause and is only reached after the result row is successfully fetched and parsed, so a cancel/deadline never surfaces here. The malformed-response sites in the same function are intentionally left untagged (distinct concern). Adds a classifyError test for the from->to. Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>
Group B — wrap a raw error through a driver constructor, then tag it: - decompression_error: a CloudFetch payload that fails LZ4 decompression (internal/rows/arrowbased/batchloader.go) is now wrapped in a driver error and tagged. It reaches telemetry via the same row-iteration path as chunk_download_error. Previously the raw lz4 error was re-wrapped downstream into a misleading "row number not contained" message and classified as invalid_request/error. - execute_statement_cancelled: a query cancelled during status polling (internal/backend/thrift/backend.go pollOperation) is tagged only when the cause is context.Canceled. context.DeadlineExceeded is intentionally left untagged so it keeps its existing classification. This is the live cancellation site (the operation has a statement id here); the raw ctx.Err() from executeStatement's cancel return is not tagged because it carries no statement id and never reaches a telemetry hook. Both wraps preserve errors.Is (including errors.Is(err, context.Canceled) for the thrift layer). Tests cover the decompression tag end-to-end through the batch iterator, and the cancel vs deadline branch in pollOperation. The arrow-IPC schema-parse errors in arrowRecordIterator are deliberately not tagged: that is the public GetArrowBatches path, whose errors go straight to the application and never reach classifyError (a caller-only site). The live Arrow schema path is already tagged separately. 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
force-pushed
the
prathamesh/PECOBLR-3537-tag-remaining-sites
branch
from
July 22, 2026 11:59
fa409af to
b7ea558
Compare
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>
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
1 participant
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
Tags the following error sources so telemetry reports
a precise
error_nameinstead of the genericerrorfallback:unsupported_operationconnection.go)decompression_errorinternal/rows/arrowbased/batchloader.go)execute_statement_cancelledcontext.Canceledonly (internal/backend/thrift/backend.go)decompression_errorwraps the raw lz4 error through a driver constructor (Group Bpattern) — previously it was re-wrapped downstream into a misleading "row number
not contained" message and misclassified.
execute_statement_cancelledtagsonly
context.Canceled;context.DeadlineExceededis left untagged so itkeeps its existing classification.
Why it's safe
WithCategorychaining (same concrete pointer), soerrors.Is/errors.As, thedbsqlerr.DBErrorassertion in the Arrow scanpath,
Error()strings, and sentinel identity are unchanged. The cancellationwrap preserves
errors.Is(err, context.Canceled)for the thrift layer.classifyErroron a verified live path; tests cover thedecompression tag end-to-end through the batch iterator and the cancel-vs-deadline
branch in
pollOperation.Out of scope
The arrow-IPC schema-parse errors in
arrowRecordIteratorare deliberately leftuntagged, that's the public
GetArrowBatchespath, whose errors go straight tothe application and never reach
classifyError.Stacked on #415 / the Group-A PR; base is the Group-A branch, auto-retargets to
mainas the earlier PRs merge.Design doc: https://docs.google.com/document/d/12ufP1eZrgFxWt6xzINfkhfrE-NnhB29zCa5PKokVfp8/edit
Jira: PECOBLR-3537