Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(mimirtool): Make remote-read actually return data #10286

Merged
merged 5 commits into from
Dec 19, 2024

Conversation

julienduchesne
Copy link
Member

@julienduchesne julienduchesne commented Dec 19, 2024

Since #9156 probably, all the remote read commands return no data

I added a missing error return using timeseries.Err() and got this error: mimirtool: error: chunkedReader: message size exceeded the limit 0 bytes; got: 361 bytes, try --help which lead me to find that we need to pass in ChunkedReadLimit on the client

I set it at a default 1 MiB and configurable through a flag

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

Since #9156 probably, all the remote read commands return no data

I added a missing error return using `timeseries.Err()` and got this error: `mimirtool: error: chunkedReader: message size exceeded the limit 0 bytes; got: 361 bytes, try --help`
which lead me to find that we need to pass in `ChunkedReadLimit` on the client

I set it at a default 1 MiB and configurable through a flag
@julienduchesne julienduchesne force-pushed the julienduchesne/mimirttool-real branch from a3fcceb to dfceee9 Compare December 19, 2024 14:49
@julienduchesne julienduchesne marked this pull request as ready for review December 19, 2024 14:50
@julienduchesne julienduchesne requested a review from a team as a code owner December 19, 2024 14:50
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1MiB is also what mimir uses by default

// Maximum number of bytes in frame when using streaming remote read.
// Google's recommendation is to keep protobuf message not larger than 1MB.
// https://developers.google.com/protocol-buffers/docs/techniques#large-data
maxRemoteReadFrameBytes = 1024 * 1024

SGTM, but maybe having something like 10-100MiB won't hurt so mimirtool is interoperable with other implementations that might choose slightly higher values.

@dimitarvdimitrov
Copy link
Contributor

I think the limit before the PR you linked was 50MiB, so we can restore that

// DefaultChunkedReadLimit is the default value for the maximum size of the protobuf frame client allows.
// 50MB is the default. This is equivalent to ~100k full XOR chunks and average labelset.
const DefaultChunkedReadLimit = 5e+7

@julienduchesne julienduchesne enabled auto-merge (squash) December 19, 2024 15:11
@julienduchesne julienduchesne merged commit 569c22d into main Dec 19, 2024
29 checks passed
@julienduchesne julienduchesne deleted the julienduchesne/mimirttool-real branch December 19, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants