Remote read first class: workflow structure #8370
Merged
Conversation
This file contains 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
Make place for a remote read roundtripper that would wrap limits and blocking checks. Update the format of how we log the remote read query matchers in query stats and activity tracker. This will enable having the same string in the logs and also the query blocker. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
krajorama
force-pushed
the
apply-middlewares-to-remote-read
branch
from
f89272f
to
381148f
Compare
krajorama
commented
Jun 14, 2024
pracucci
approved these changes
Jun 14, 2024
| })) | ||
| _, error := handler.Do(req.Context(), metricsRequest) | ||
| if error != nil { | ||
| return nil, apierror.AddDetails(error, fmt.Sprintf("remote read error (%s_%d: %s)", matchersLogKey, i, metricsRequest.GetQuery())) |
There was a problem hiding this comment.
I don't want to block on this, and we can address it separately, but I'm not 💯 convinced about the AddDetails() here. The design smell to me is that it's very easy to forget to use AddDetails() instead of the generic errors.Wrap(). I would like to make this stuff work with the normal errors.Wrap().
There was a problem hiding this comment.
I wasn't sure that's a good idea as our Wrap would behave differently than the standard one (not in terms of the string returned, but the structure), but I'll update in the next PR.
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
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.
What this PR does
Make place for a remote read roundtripper that would wrap limits and blocking checks.
Update the format of how we log the remote read query matchers in query stats and activity tracker. This will enable having the same string in the logs and also the query blocker.
Which issue(s) this PR fixes or relates to
Relates to https://github.com/grafana/pir-actions/issues/10 (internal, make remote read first class)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.