Tag CloudFetch, Arrow schema, and result-fetch errors for telemetry#417
Draft
prathamesh-db wants to merge 1 commit into
Draft
Conversation
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>
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
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().Stacked on #415 (base is that branch, not
main); auto-retargets tomainonce #415 merges.Design doc: https://docs.google.com/document/d/12ufP1eZrgFxWt6xzINfkhfrE-NnhB29zCa5PKokVfp8/edit
Jira: PECOBLR-3537