-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Remote Write: Add max samples per metadata send #8959
Remote Write: Add max samples per metadata send #8959
Conversation
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few small comments.
storage/remote/queue_manager.go
Outdated
if err != nil { | ||
t.metrics.failedMetadataTotal.Add(float64(len(metadata))) | ||
level.Error(t.logger).Log("msg", "non-recoverable error while sending metadata", "count", len(metadata), "err", err) | ||
var i int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could just assign i := math.Ceil(len(metadata)/t.mcfg.MaxSamplesPerSend))
and then keep track of the start and end indexes into the slice for deciding what samples to send? Or something like that. Then we don't need the if statement you have below to check if there were remaining samples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep track of the start and end indexes into the slice for deciding what samples to send
I'm not sure what you mean here.
Also, math.Ceil
isn't needed because the decimals are implicitly truncated when assigning to an int
(and we don't want to round, just truncate).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying we should maintain the decimal places (cast one of the two ints to a float) so that we can round up if len(metadata)
is not evenly divisible by MaxSamplesPerSend
. Either that or you could use len(metadata) % MaxSamplesPerSend
and increment i
if the remainder was not 0.
keep track of the start and end indexes into the slice for deciding what samples to send
I meant you'd need to a way to track what indexes within the metadata slice are a part of the next metadata send, just so that on the last send we index slice[beginning of last send:end of slice]
instead of slice[beginning of last send:beginning plus MaxSamplesPerSend]
Either way, this would get rid of the need for the if condition at the end that checks if we need to do one last send.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hows this:
for i = 0; i < int(math.Ceil(float64(len(metadata))/float64(t.mcfg.MaxSamplesPerSend)); i++ {
last := (i+1)*t.mcfg.MaxSamplesPerSend
if last > len(metadata) {
last = len(metadata)
}
err := t.sendMetadataWithBackoff(ctx, mm[i*t.mcfg.MaxSamplesPerSend:last])
if err != nil {
t.metrics.failedMetadataTotal.Add(float64(t.mcfg.MaxSamplesPerSend))
level.Error(t.logger).Log("msg", "non-recoverable error while sending metadata", "count", t.mcfg.MaxSamplesPerSend, "err", err)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would put the math.Ceil
calculation on it's own line with something like numSends := math.Ceil(...)
but this looks good 👍
storage/remote/queue_manager_test.go
Outdated
@@ -522,6 +531,7 @@ type TestWriteClient struct { | |||
receivedExemplars map[string][]prompb.Exemplar | |||
expectedExemplars map[string][]prompb.Exemplar | |||
receivedMetadata map[string][]prompb.MetricMetadata | |||
writes int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we give this a better name? expectedNumWrites
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think expectedNumWrites
is the proper term, because it's not a specified expectation, it's how many are counted.
How about writesReceived
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, sounds good 👍
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks @LeviHarrison |
Thanks for your time reviewing! |
Why is it named "max_samples" when it's sending metadata, not samples? |
I guess because it's a subfield of |
This PR adds the
max_samples_per_send
option under themetadata_config
.Fixes #8870