Repository navigation
Conversation
collector-timeouts uses errors.Is(err, context.DeadlineExceeded), which only matches when collectors keep the error chain. trx, stmt.current, wait.io.table and the repl.lag PFS path wrapped query errors with %s or %v, so their CMR timeouts weren't counted. Wrap with %w, keeping the message text, and drop the docs caveat for trx and stmt.current. Add mock.ErrorDB, a *sql.DB whose connections always fail with a given error, plus tests that each changed collector keeps the context.DeadlineExceeded chain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
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.
Stacked on #187: review against
Areson/collection-fault-counters.#187 counts
collector-timeoutswitherrors.Is(err, context.DeadlineExceeded), but some built-in collectors wrap query errors with%sor%v. That flattens the error chain, so when those collectors hit their CMR deadline the timeout isn't counted. This PR changes those wraps to%w. The message text stays the same.Wraps changed in
Collectpaths:metrics/trx/trx.go: oldest-trx query error (%s→%w)metrics/stmt.current/current.go: query error and row scan error (%s→%w, two places)metrics/wait.io.table/table.go:rows.Columns()error (%v→%w)metrics/repl.lag/pfs.go: replica check query error and lag query error (%switherr.Error()→%w, two places)The
trxandstmt.currentchanges were flagged in review on #187. I found thewait.io.tableandrepl.lagones by greppingmetrics/for%sand%vwraps. Every other match is inPrepareor option parsing, where CMR doesn't apply, so I left those alone.With this change, the "
trxandstmt.currentdon't yet" caveat comes out ofdocs/content/api/status.md.Tests:
mock.ErrorDB(test/mock/db.go). It returns a*sql.DBwhose connections always fail with a given error, so tests can check how a collector wraps errors without MySQL.trx,stmt.currentand bothrepl.lagPFS queries. In each, the query fails with an error wrappingcontext.DeadlineExceeded, and the test assertserrors.Isstill matches afterCollectwraps it. With the old wraps, all three fail.wait.io.tableColumns()wrap has no unit test. That error only comes from rows that a context cancel closes after the query succeeds, anddatabase/sqlcloses them in a goroutine, so a test would be racy.stmt.currentscan-error wrap has no test either, for the same reason.Not changed:
collectPFSinrepl.lagruns its lag query withcontext.Background()rather thanctx, so CMR doesn't bound that query today. Fixing that changes behavior, so it should be its own change.🤖 Generated with Claude Code