feat: add failed and slow queries to query stats#27536
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the query executor’s _internal query statistics by adding two new metrics: total failed queries and a snapshot count of currently-running “slow” queries (running longer than log-queries-after). This aligns runtime monitoring with existing slow-query logging semantics and provides better observability for query failures.
Changes:
- Add
queriesFailedstat and increment it based on terminal errors, execution-context-emitted errors, and panics. - Add
queriesSlowstat by computing an in-flight slow-query snapshot from theTaskManagerusingLogQueriesAfter. - Add unit tests covering
queriesFailedcounting behavior andSlowQueryCount()behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
query/task_manager.go |
Adds SlowQueryCount() to snapshot in-flight queries exceeding LogQueriesAfter. |
query/executor.go |
Adds queriesFailed / queriesSlow stats and wires counting into execution + panic recovery paths. |
query/executor_test.go |
Adds tests for queriesFailed semantics and slow-query snapshot counting. |
query/execution_context.go |
Tracks whether an error Result has been sent/attempted via a new atomic failed flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
query/executor.go:394
- This error Result is sent directly on
resultswithout settingStatementID(so multi-statement queries can report the error against statement 0) and without honoringAbortCh(so a caller aborting can still leave this goroutine blocked on send). Usectx.sendwithStatementID: iso AbortCh is respected and the statement index is correct.
failed = true
results <- &Result{
Err: fmt.Errorf("unable to use system source '%s': use %s instead", s.Name, command),
}
break LOOP
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
devanbenz
left a comment
There was a problem hiding this comment.
Just one comment about failed vs cancelled/interrupted queries.
| } | ||
|
|
||
| if interrupted { | ||
| // A query killed or timed out between statements is a failed query, |
There was a problem hiding this comment.
Should we consider a cancelled query as failed? Or should we use a different metric for this i.e. CancelledQueries?
There was a problem hiding this comment.
I think canceled queries will be such a small percentage, we can consider them failed because of user unhappiness.
Add
statQueriesFailed = "queriesFailed" // Number of queries that have failed.
statQueriesSlow = "queriesSlow" // Snapshot of in-flight queries running longer than log-queries-after.
Closes https://github.com/influxdata/feature-requests/issues/392