feat: surface the flush-failure cause in the retry log#26
Merged
Conversation
"quickwit: flush failed, retrying indefinitely" logged only an attempt counter, so an operator watching an ingest stall could not tell a transport error / timeout from a 5xx or a backpressure 429/503 — the two silent return-false sites (http.NewRequest and httpClient.Do) dropped the error entirely. Carry the most recent failure out of flush() into retryFlush() via a per-worker lastErr, mirroring the existing retryAfter pattern (reset at the top of every flush, set at each transient return-false site), and add it as the "error" attribute on both retry log lines. No behavior change beyond the log content.
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.
Problem
quickwit: flush failed, retrying indefinitelylogged only anattemptcounter. When an ingest stream stalls (observed live: hundreds of these/hour,attemptclimbing to 20, with a multi-million-message pubsub backlog upstream), there was no way to tell from the log what actually failed — a transport error / timeout, a 5xx, or a backpressure 429/503. Worse, the two most common failure sites inflush()—http.NewRequestWithContextandhttpClient().Do— didreturn falseand dropped the error entirely, so a network-level Quickwit outage produced pureattempt=Nnoise.Change
Carry the most recent failure reason out of
flush()intoretryFlush()via a per-workerlastErr, mirroring the existingretryAfterpattern:retryAfter, reset at the top of every flush (so it only reflects the latest attempt),return falsesite (gzip encode,NewRequest,Do, non-2xx status, response read),"error"attribute on both retry log lines (retrying indefinitelyandretrying while closing).No behavior change beyond the log content.
lastErris a per-worker stack local likeretryAfter, so no added cross-goroutine sharing.Before:
After (examples):
Tests
TestFlushFailure_RetryLogIncludesCause: captures slog output, drives a server that returns two 500s then 200, and asserts the retry line carries anerrorattr naming the500status.go vet,go test ./...(66), andgo test -race ./...all green.