querymiddleware: extract error message into exported constant #13485
Conversation
…nseStr constant to avoid duplication and allow external services to reference it for retry logic.
|
|
sherinabr
added
the
changelog-not-needed
|
💻 Deploy preview available (querymiddleware: extract error message into exported constant): |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
56quarters
left a comment
56quarters
left a comment
There was a problem hiding this comment.
There's a lot of unrelated changes here and I don't think the core change of inspecting error messages is a robust approach for retries.
|
|
||
| const ( | ||
| // ErrMergingPartialResponseStr is the error message used when merging partial responses from sharded queries fails. | ||
| // This is used by the recommendations service (backend-enterprise repo) to determine whether to retry requests. |
There was a problem hiding this comment.
I don't think we should be inspecting error messages to determine if requests should be retried. The query-frontend sets reasonable HTTP status codes and the APIErrors returned set a type that can be used to determine if something is transient or not.
There was a problem hiding this comment.
Sorry for the delayed response.
This PR was created because the Active Series API sometimes returns an HTTP 200 (with no error) along with a partial result. Since no error is returned, the recommendations engine proceeds with the incomplete data and computes the series counts, which leads to incorrect values being shown in the UI.
I’ve started a Slack thread with more context. If Active Series API could return an error in this case, we wouldn't need any of these changes.
sherinabr
force-pushed
the
sherin/use-error-string-for-error-merging-partial-responses
branch
2 times, most recently
from
c2044ec to
ca6a63a
Compare
…-string-for-error-merging-partial-responses
3 participants
What this PR does
Extract the hardcoded "error merging partial responses" string into an exported constant
ErrMergingPartialResponseStrinshard_by_series_base.go.Which issue(s) this PR fixes or relates to
N/A
Fixes #
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.Note
Introduces
ErrMergingPartialResponseStrand replaces hardcoded "error merging partial responses" strings in middlewares, unifying log and response error formatting.ErrMergingPartialResponseStrinpkg/frontend/querymiddleware/shard_by_series_base.go.shard_active_series.goandshard_active_native_histogram_metrics.go."%s: %s"format using the constant for both logs and JSON responses.Written by Cursor Bugbot for commit 0939782. This will update automatically on new commits. Configure here.