Tag session-close, statement-close, and rate-limit errors for telemetry - #424
Merged
Merged
Conversation
prathamesh-db
force-pushed
the
prathamesh/PECOBLR-3537-tag-caller-sites
branch
from
July 23, 2026 05:11
4a03e2a to
f8afecc
Compare
Tags the remaining live error sources so telemetry reports a specific error_name instead of the generic "error": - session_closed: a failed CloseSession is tagged on a telemetry-only copy in conn.Close before RecordOperation(DELETE_SESSION). The returned error is left untouched so it stays a driver.ErrBadConn (database/sql relies on that for connection-pool eviction). - statement_closed: a failed CloseOperation RPC is tagged via a telemetry-only helper at the three RecordOperation(CLOSE_STATEMENT) sites (sync exec + the two async close-callback paths). The raw error still flows to the caller/rows unchanged. - rate_limit_exceeded: an HTTP 429 that survives all retries is tagged in the retry errorHandler, which is the only place the 429 status is visible (net/http discards the response on a transport error). Scoped to 429 exactly, so 503 keeps its existing classification. This covers the retry-exhausted failure only; a per-attempt 429 counter metric is left as a TODO follow-up. All tags are telemetry-only and use WithCategory (same concrete pointer), so errors.Is/As, driver.ErrBadConn identity, Error() strings, and the returned errors are unchanged. Tests assert each tagged error yields the right category and that 503/500 are not tagged rate_limit and the returned session-close error is still driver.ErrBadConn. Signed-off-by: Prathamesh Baviskar <prathamesh.baviskar@databricks.com>
prathamesh-db
force-pushed
the
prathamesh/PECOBLR-3537-tag-caller-sites
branch
from
July 23, 2026 12:09
f8afecc to
96c8029
Compare
prathamesh-db
marked this pull request as ready for review
July 26, 2026 14:30
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.
make sure you test this e2e as discussed - 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>
Base automatically changed from
prathamesh/PECOBLR-3537-tag-remaining-sites
to
main
July 29, 2026 04:43
prathamesh-db
enabled auto-merge
July 29, 2026 04:49
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
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
Tags the remaining live error sources so telemetry reports a specific
error_nameinstead of the genericerror:session_closedCloseSession(connection.go, DELETE_SESSION telemetry)statement_closedCloseOperationRPC (connection.go, the 3 CLOSE_STATEMENT sites)rate_limit_exceededinternal/client/client.goretryerrorHandler)All tags are telemetry-only and use the existing
WithCategorychaining(same concrete pointer), so
errors.Is/errors.As,Error()strings, and theerrors returned to callers are unchanged:
session_closedtags a separate telemetry copy; the returned error stays adriver.ErrBadConn(database/sqlrelies on that for connection-pool eviction).statement_closedtags only the telemetry argument; the raw close error stillflows to the caller/rows.
rate_limit_exceededis tagged in the retryerrorHandler— the only place the429 status is visible (net/http discards the response on a transport error).
Scoped to 429 exactly, so 503 keeps its existing classification. This covers the
retry-exhausted failure only; a per-attempt 429 counter metric is a TODO follow-up.
Notes for reviewers (by design)
statement_closedis only live on the Thrift backend; the kernel backend's closenever returns an error, so it can't be tagged there.
a 429 reports
rate_limit_exceededrather thansession_closed/statement_closed— the more specific category.
Naming note
session_closedhas no exact JDBC equivalent (JDBC's nearest isCONNECTION_CLOSED);kept Go-specific here.
Design doc: https://docs.google.com/document/d/12ufP1eZrgFxWt6xzINfkhfrE-NnhB29zCa5PKokVfp8/edit
Jira: PECOBLR-3537