Distributor: don't return errors when discarding samples with duplicated timestamps #10430
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
…ted timestamps Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
tacole02
approved these changes
Jan 14, 2025
Co-authored-by: Taylor C <[email protected]>
pracucci
reviewed
Jan 14, 2025
pkg/distributor/distributor.go
Outdated
| @@ -899,7 +899,7 @@ func (d *Distributor) validateSeries(nowt time.Time, ts *mimirpb.PreallocTimeser | |||
| if deduplicatedSamplesAndHistograms > 0 { | |||
| d.sampleValidationMetrics.duplicateTimestamp.WithLabelValues(userID, group).Add(float64(deduplicatedSamplesAndHistograms)) | |||
| unsafeMetricName, _ := extract.UnsafeMetricNameFromLabelAdapters(ts.Labels) | |||
| return false, fmt.Errorf(duplicateTimestampMsgFormat, deduplicatedSamplesAndHistograms, unsafeMetricName) | |||
| level.Warn(d.log).Log("msg", "samples with duplicated timestamps have been discarded", "series", fmt.Sprintf("'%.200s'", unsafeMetricName), "samples discarded", deduplicatedSamplesAndHistograms) | |||
There was a problem hiding this comment.
I'm concerned we may log too much. This is per-series. In the worst case, this would be logged for every series in every write request, killing distributors.
There was a problem hiding this comment.
Hi @pracucci ,
We could then either remove logs or try to sample them, like we already do with ingesters, where in some cases we log only 10% of actual logs. WDYT?
There was a problem hiding this comment.
Hm... we actually don't sample logs but errors. So the sampling proposal cannot be done.
There was a problem hiding this comment.
We agreed to remove logs for the time being, and to introduce rate limited logs in a follow-up PR.
Signed-off-by: Yuri Nikolic <[email protected]>
pracucci
approved these changes
Jan 14, 2025
duricanikolic
added a commit
that referenced
this pull request
Jan 14, 2025
…ted timestamps (#10430) (#10434) * Distributor: don't return errors when discarding samples with duplicated timestamps Signed-off-by: Yuri Nikolic <[email protected]> * Update CHANGELOG.md Signed-off-by: Yuri Nikolic <[email protected]> * Update CHANGELOG.md Co-authored-by: Taylor C <[email protected]> * Remove logging for the time being Signed-off-by: Yuri Nikolic <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]> Co-authored-by: Taylor C <[email protected]> (cherry picked from commit 78e0d3a) Co-authored-by: Đurica Yuri Nikolić <[email protected]>
duricanikolic
added a commit
that referenced
this pull request
Jan 14, 2025
…ted timestamps (#10430) (#10437) * Distributor: don't return errors when discarding samples with duplicated timestamps Signed-off-by: Yuri Nikolic <[email protected]> * Update CHANGELOG.md Signed-off-by: Yuri Nikolic <[email protected]> * Update CHANGELOG.md Co-authored-by: Taylor C <[email protected]> * Remove logging for the time being Signed-off-by: Yuri Nikolic <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]> Co-authored-by: Taylor C <[email protected]> (cherry picked from commit 78e0d3a) Co-authored-by: Đurica Yuri Nikolić <[email protected]>
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
In #10145 we introduced a request validation at distributor level that, for each object of type
mimirpb.PreallocTimeseriesin a request, iterates all its float and histogram samples, and as soon as a sample with a timestamp that has already been visited is found, discards the corresponding sample from the timeseries. Once the validation is completed, the request without discarded samples is forwarded to ingesters. Discarded samples are tracked by thecortex_discarded_samples_totalmetrics with reasonsample_duplicate_timestamp.One of the problems introduced by #10145 is that if a distributor discards at least one sample due to a duplicated timestamp, it also returns an error reporting the name of the timeseries, and the number of discarded samples. This behaviour introduces a confusion since
In order to remove this ambiguity, we decided to:
mimirpb.PreallocTimeseriesobject on distributor side.cortex_discarded_samples_totalmetrics with reasonsample_duplicate_timestamp.Which issue(s) this PR fixes or relates to
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.