Repository navigation
Add type label to cortex_discarded_samples_total to distinguish float and native histogram samples - #7898
Add type label to cortex_discarded_samples_total to distinguish float and native histogram samples#7898Sun-Wukhan wants to merge 10 commits into
type label to cortex_discarded_samples_total to distinguish float and native histogram samples#7898Conversation
Distinguish discarded float samples from discarded native histogram samples in the cortex_discarded_samples_total metric. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Distinguish discarded float samples from discarded native histogram samples in the cortex_discarded_samples_total metric. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Distinguish discarded float samples from discarded native histogram samples in the cortex_discarded_samples_total metric. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com> Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Distinguish discarded float samples from discarded native histogram samples in the cortex_discarded_samples_total metric. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Distinguish discarded float samples from discarded native histogram samples in the cortex_discarded_samples_total metric. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Distinguish discarded float samples from discarded native histogram samples in the cortex_discarded_samples_total metric. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
sandy2008
left a comment
There was a problem hiding this comment.
Thanks for working on this. The main issue is that several paths shared by floats and native histograms now always record type="float", so histogram discards still land in the float series, which is what #6221 asks to separate (details inline). Separately, the test / test-no-race failures come from this change: the two TestIngester_Push cases added in #7887 (ooo disabled, rule result out of order is counted as a discarded sample and ooo disabled, tenant staleness marker out of order is counted as a discarded sample, at pkg/ingester/ingester_test.go:1515 and :1567) still expect cortex_discarded_samples_total{reason="sample-out-of-order",user="test"} without the new label. I reproduced both failures locally with go test -run TestIngester_Push ./pkg/ingester/.
| if sampleOutOfBoundsCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(sampleOutOfBounds, userID).Add(float64(sampleOutOfBoundsCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(sampleOutOfBounds, userID, validation.SampleTypeFloat).Add(float64(sampleOutOfBoundsCount)) | ||
| } | ||
| if sampleOutOfOrderCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(sampleOutOfOrder, userID).Add(float64(sampleOutOfOrderCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(sampleOutOfOrder, userID, validation.SampleTypeFloat).Add(float64(sampleOutOfOrderCount)) | ||
| } | ||
| if sampleTooOldCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(sampleTooOld, userID).Add(float64(sampleTooOldCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(sampleTooOld, userID, validation.SampleTypeFloat).Add(float64(sampleTooOldCount)) | ||
| } | ||
| if newValueForTimestampCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(newValueForTimestamp, userID).Add(float64(newValueForTimestampCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(newValueForTimestamp, userID, validation.SampleTypeFloat).Add(float64(newValueForTimestampCount)) | ||
| } | ||
| if perUserSeriesLimitCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perUserSeriesLimit, userID).Add(float64(perUserSeriesLimitCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perUserSeriesLimit, userID, validation.SampleTypeFloat).Add(float64(perUserSeriesLimitCount)) | ||
| } | ||
| if perUserNativeHistogramSeriesLimitCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perUserNativeHistogramSeriesLimit, userID).Add(float64(perUserNativeHistogramSeriesLimitCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perUserNativeHistogramSeriesLimit, userID, validation.SampleTypeHistogram).Add(float64(perUserNativeHistogramSeriesLimitCount)) | ||
| } | ||
| if perMetricSeriesLimitCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perMetricSeriesLimit, userID).Add(float64(perMetricSeriesLimitCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perMetricSeriesLimit, userID, validation.SampleTypeFloat).Add(float64(perMetricSeriesLimitCount)) | ||
| } | ||
| if perLabelSetSeriesLimitCount > 0 { | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perLabelsetSeriesLimit, userID).Add(float64(perLabelSetSeriesLimitCount)) | ||
| i.validateMetrics.DiscardedSamples.WithLabelValues(perLabelsetSeriesLimit, userID, validation.SampleTypeFloat).Add(float64(perLabelSetSeriesLimitCount)) |
There was a problem hiding this comment.
handleAppendFailure is used for both float samples (line 1637) and native histograms (line 1695), so these counters mix the two kinds and everything is reported as float here. For example, the native histogram ooo enabled, should soft fail on sample too old case only pushes histograms but now expects type="float". Keeping separate float and histogram counters (e.g. by telling handleAppendFailure which kind it is handling) would fix it.
| # HELP cortex_discarded_samples_total The total number of samples that were discarded. | ||
| # TYPE cortex_discarded_samples_total counter | ||
| cortex_discarded_samples_total{reason="sample-too-old",user="test"} 1 | ||
| cortex_discarded_samples_total{reason="sample-too-old",type="float",user="test"} 1 |
There was a problem hiding this comment.
This case only pushes native histograms, so the rejected sample should be counted under type="histogram"; as written, the expectation locks in the mislabel from the ingester change above.
| if limits.RejectOldSamples && model.Time(timestampMs) < model.Now().Add(-time.Duration(limits.RejectOldSamplesMaxAge)) { | ||
| validateMetrics.DiscardedSamples.WithLabelValues(greaterThanMaxSampleAge, userID).Inc() | ||
| validateMetrics.DiscardedSamples.WithLabelValues(greaterThanMaxSampleAge, userID, SampleTypeFloat).Inc() | ||
| return newSampleTimestampTooOldError(unsafeMetricName, timestampMs) | ||
| } | ||
|
|
||
| if model.Time(timestampMs) > model.Now().Add(time.Duration(limits.CreationGracePeriod)) { | ||
| validateMetrics.DiscardedSamples.WithLabelValues(tooFarInFuture, userID).Inc() | ||
| validateMetrics.DiscardedSamples.WithLabelValues(tooFarInFuture, userID, SampleTypeFloat).Inc() |
There was a problem hiding this comment.
ValidateSampleTimestamp is also called for every native histogram (pkg/distributor/distributor.go:730), so histograms rejected as too old or too far in the future are counted as float. Passing the sample type in from the caller would keep these accurate.
| numLabelNames := len(ls) | ||
| if numLabelNames > limits.MaxLabelNamesPerSeries { | ||
| validateMetrics.DiscardedSamples.WithLabelValues(maxLabelNamesPerSeries, userID).Inc() | ||
| validateMetrics.DiscardedSamples.WithLabelValues(maxLabelNamesPerSeries, userID, SampleTypeFloat).Inc() |
There was a problem hiding this comment.
ValidateLabels rejects the whole series, which can contain only native histograms, but these reasons are always recorded as float. It probably needs the same treatment as the timestamp check, e.g. taking the type (or the per-type counts) from the caller.
|
|
||
| ## master / unreleased | ||
|
|
||
| * [ENHANCEMENT] Distributor/Ingester: Add `type` label to `cortex_discarded_samples_total` metric to distinguish between float and native histogram samples. #6221 |
There was a problem hiding this comment.
Nit: the entry should end with the PR number (#7898) rather than the issue. Also, since this changes the label set of an existing metric, [CHANGE] might fit better (it's how the type label on cortex_distributor_ingester_appends_total was recorded), which would also put it ahead of the [FEATURE] entries.
Thread the sample type through the paths shared by float and native histogram samples so histogram discards are no longer recorded as float: the ingester append-failure handler now splits its per-reason counters by type, ValidateSampleTimestamp takes the sample type from its caller, and ValidateLabels attributes rejections to each sample type present in the series. Also correct the TestIngester_Push expectations that locked in the mislabel, and record the change as [CHANGE] citing cortexproject#7898 in the CHANGELOG. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Thread the sample type through the paths shared by float and native histogram samples so histogram discards are no longer recorded as float: the ingester append-failure handler now splits its per-reason counters by type, ValidateSampleTimestamp takes the sample type from its caller, and ValidateLabels attributes rejections to each sample type present in the series. Also correct the TestIngester_Push expectations that locked in the mislabel, and record the change as [CHANGE] citing cortexproject#7898 in the CHANGELOG. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Thread the sample type through the paths shared by float and native histogram samples so histogram discards are no longer recorded as float: the ingester append-failure handler now splits its per-reason counters by type, ValidateSampleTimestamp takes the sample type from its caller, and ValidateLabels attributes rejections to each sample type present in the series. Also correct the TestIngester_Push expectations that locked in the mislabel, and record the change as [CHANGE] citing cortexproject#7898 in the CHANGELOG. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
Thread the sample type through the paths shared by float and native histogram samples so histogram discards are no longer recorded as float: the ingester append-failure handler now splits its per-reason counters by type, ValidateSampleTimestamp takes the sample type from its caller, and ValidateLabels attributes rejections to each sample type present in the series. Also correct the TestIngester_Push expectations that locked in the mislabel, and record the change as [CHANGE] citing cortexproject#7898 in the CHANGELOG. Signed-off-by: Navid Khan (Ali) <60997541+Sun-Wukhan@users.noreply.github.com>
|
Thanks for the careful review. All five points are addressed in the latest commits: the shared ingester and validation paths now receive the sample type from their callers, ValidateLabels attributes a series rejection to each sample type present in the series, the histogram test case expects type="histogram", and the CHANGELOG entry now cites #7898 as a [CHANGE]. The affected package tests pass locally. |
Fixes #6221
cortex_discarded_samples_totalcurrently aggregates discarded float samples and discarded native histogram samples into the same series, so operators cannot tell which kind is being discarded.This adds a
typelabel (float/histogram) to the metric inpkg/util/validationand threads it through everyDiscardedSamplescall site in the distributor and ingester, splitting combined float+histogram counts where both kinds can be discarded together.cortex_discarded_samples_per_labelset_totalis unchanged. A CHANGELOG entry is included.Disclosure per GENAI_POLICY.md: this change was prepared with AI assistance and has been reviewed and verified by me. Focused tests pass locally (
pkg/util/validation, and the ingester per-labelset limit test that asserts this metric).