Skip to content

Commit 2630d40

Browse files
committed
Address PR #321 review comments: Fix ExecContext telemetry timing and CloseOperation error tracking
This commit addresses two key review comments from @vikrantpuppala on PR #321: 1. **ExecContext timing fix**: Capture execution start time BEFORE running query - Now captures `executeStart := time.Now()` before `runQuery()` call - Uses `BeforeExecuteWithTime()` with pre-captured timestamp - Matches the pattern already implemented in QueryContext - Ensures telemetry accurately measures actual query execution time 2. **CloseOperation error tracking**: Capture cleanup errors in telemetry - Added `closeOpErr` variable to track CloseOperation failures - Includes CloseOperation errors in telemetry's deferred function - Provides observability for resource cleanup issues - Operation still returns success to caller (cleanup is best-effort) These changes ensure telemetry captures the complete statement lifecycle, including both execution timing and cleanup operations, without impacting the caller's error handling semantics. Co-authored-by: Isaac
1 parent c6e3b2a commit 2630d40

1 file changed

Lines changed: 11 additions & 1 deletion

File tree

connection.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,25 +125,34 @@ func (c *conn) ExecContext(ctx context.Context, query string, args []driver.Name
125125

126126
corrId := driverctx.CorrelationIdFromContext(ctx)
127127

128+
// Capture execution start time for telemetry before running the query
129+
executeStart := time.Now()
128130
var pollCount int
129131
exStmtResp, opStatusResp, err := c.runQuery(ctx, query, args, &pollCount)
130132
log, ctx = client.LoggerAndContext(ctx, exStmtResp)
131133
stagingErr := c.execStagingOperation(exStmtResp, ctx)
132134

133135
// Telemetry: track statement execution
134136
var statementID string
137+
var closeOpErr error // Track CloseOperation errors for telemetry
135138
if c.telemetry != nil && exStmtResp != nil && exStmtResp.OperationHandle != nil && exStmtResp.OperationHandle.OperationId != nil {
136139
statementID = client.SprintGuid(exStmtResp.OperationHandle.OperationId.GUID)
137-
ctx = c.telemetry.BeforeExecute(ctx, c.id, statementID)
140+
// Use BeforeExecuteWithTime to set the correct start time (before execution)
141+
ctx = c.telemetry.BeforeExecuteWithTime(ctx, c.id, statementID, executeStart)
138142
defer func() {
139143
finalErr := err
140144
if stagingErr != nil {
141145
finalErr = stagingErr
142146
}
147+
// Include CloseOperation error in telemetry if it occurred
148+
if closeOpErr != nil && finalErr == nil {
149+
finalErr = closeOpErr
150+
}
143151
c.telemetry.AfterExecute(ctx, finalErr)
144152
c.telemetry.CompleteStatement(ctx, statementID, finalErr != nil)
145153
}()
146154
c.telemetry.AddTag(ctx, "poll_count", pollCount)
155+
c.telemetry.AddTag(ctx, "operation_type", telemetry.OperationTypeExecuteStatement)
147156
}
148157

149158
if exStmtResp != nil && exStmtResp.OperationHandle != nil {
@@ -156,6 +165,7 @@ func (c *conn) ExecContext(ctx context.Context, query string, args []driver.Name
156165
})
157166
if err1 != nil {
158167
log.Err(err1).Msg("databricks: failed to close operation after executing statement")
168+
closeOpErr = err1 // Capture for telemetry
159169
}
160170
}
161171
}

0 commit comments

Comments
 (0)