Add metric for number of series with discarded samples #6996

eeldaly · 2025-08-28T20:56:17Z

What this PR does:
Adds a metric that counts how many series have discarded samples in them. This includes a label with the reason the sample was discarded.

Which issue(s) this PR fixes:
Fixes #6995

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

yeya24 · 2025-08-28T22:23:01Z

pkg/util/discardedseries/tracker.go

+	"per_user_series_limit",
+	"per_labelset_series_limit",
+	"per_metric_series_limit",
+}


Can we avoid hardcoding the discarded reason. There can be more reasons in the future and it is very easy to miss it

yeya24 · 2025-08-28T22:23:35Z

pkg/util/validation/validate.go

@@ -87,6 +88,8 @@ type ValidateMetrics struct {

 	DiscardedSamplesPerLabelSet *prometheus.CounterVec
 	LabelSetTracker             *labelset.LabelSetTracker
+	DiscardedSeriesGauge        *prometheus.GaugeVec


Let's avoid having metric type Gauge in the name

yeya24 · 2025-08-28T22:24:05Z

pkg/util/validation/validate.go

@@ -145,6 +148,14 @@ func NewValidateMetrics(r prometheus.Registerer) *ValidateMetrics {
 		NativeHistogramMinResetDuration: 1 * time.Hour,
 	}, []string{"user"})
 	registerCollector(r, labelSizeBytes)
+	discardedSeriesGauge := prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Name: "cortex_discarded_series_total",


_total suffix is used for counter mainly. We can remove that

Isn't the intended metric a counter? It seems to me that this metric is a counter.

yeya24 · 2025-08-28T22:24:43Z

pkg/util/validation/validate.go

@@ -434,4 +447,7 @@ func DeletePerUserValidationMetrics(validateMetrics *ValidateMetrics, userID str
 	if err := util.DeleteMatchingLabels(validateMetrics.LabelSizeBytes, filter); err != nil {
 		level.Warn(log).Log("msg", "failed to remove cortex_label_size_bytes metric for user", "user", userID, "err", err)
 	}
+	if err := util.DeleteMatchingLabels(validateMetrics.DiscardedSeriesGauge, filter); err != nil {


Can we add this to the existing test case of cleaning up user metrics?

yeya24 · 2025-08-28T22:27:22Z

pkg/util/discardedseries/tracker.go

+	return &DiscardedSeriesTracker{labelUserMap: labelUserMap, discardedSeriesGauge: discardedSeriesGauge}
+}
+
+func (t *DiscardedSeriesTracker) Track(label string, user string, series uint64) {


label is a bit confusing. Can we call it reason?

yeya24 · 2025-08-28T22:28:57Z

pkg/util/discardedseries/tracker.go

+	go func() {
+		for {
+			time.Sleep(vendMetricsInterval)
+			t.UpdateMetrics()


can we use time.Ticker?

Also let's rename StartDiscardedSeriesGoroutine. This is not a good method name. I don't see where it is being called as well

danielblando · 2025-08-28T23:20:56Z

pkg/ingester/ingester.go

 			// Check if the error is a soft error we can proceed on. If so, we keep track
 			// of it, so that we can return it back to the distributor, which will return a
 			// 400 error to the client. The client (Prometheus) will not retry on 400, and
 			// we actually ingested all samples which haven't failed.
 			switch cause := errors.Cause(err); {
 			case errors.Is(cause, storage.ErrOutOfBounds):
 				sampleOutOfBoundsCount++
+				i.validateMetrics.DiscardedSeriesTracker.Track("sample_out_of_bounds", userID, seriesHash)


We probably can use the same const we use for the DiscardedSamples metric.

cortex/pkg/ingester/series.go

Lines 3 to 9 in 746b40e

const (

sampleOutOfOrder = "sample-out-of-order"

newValueForTimestamp = "new-value-for-timestamp"

sampleOutOfBounds = "sample-out-of-bounds"

sampleTooOld = "sample-too-old"

nativeHistogramSample = "native-histogram-sample"

)

and

cortex/pkg/ingester/user_state.go

Lines 18 to 23 in 746b40e

const (

perUserSeriesLimit = "per_user_series_limit"

perUserNativeHistogramSeriesLimit = "per_user_native_histogram_series_limit"

perMetricSeriesLimit = "per_metric_series_limit"

perLabelsetSeriesLimit = "per_labelset_series_limit"

)

danielblando · 2025-08-28T23:40:54Z

pkg/util/discardedseries/tracker.go

+func NewDiscardedSeriesTracker(discardedSeriesGauge *prometheus.GaugeVec) *DiscardedSeriesTracker {
+	labelUserMap := make(map[string]*UserCounter)
+	for _, label := range trackedLabels {
+		labelUserMap[label] = &UserCounter{


Should we create when receiving a new reason? On track it seems we ignore reason not initially created

danielblando · 2025-08-28T23:46:32Z

pkg/util/discardedseries/tracker.go

+				RWMutex:        &sync.RWMutex{},
+				seriesCountMap: make(map[uint64]struct{}),
+			}
+			userCounter.Lock()


Do we need the lock to be before the check? In this case we would still override the content, no?

danielblando · 2025-08-28T23:48:59Z

pkg/util/discardedseries/tracker.go

+
+func (t *DiscardedSeriesTracker) UpdateMetrics() {
+	for label, userCounter := range t.labelUserMap {
+		userCounter.Lock()


We maybe want to consider changing it to be UserLabel instead of LabelUser.
When emitting this metric for a Label, we would block global requests from every user.
It seems this would have faster lock if we have UserLabel.

yeya24 · 2025-08-29T23:25:14Z

pkg/util/discardedseries/tracker.go

+}
+
+func (t *DiscardedSeriesTracker) Track(reason string, user string, series uint64) {
+	userCounter, ok := t.labelUserMap[reason]


We need a lock here?

yeya24 · 2025-08-29T23:25:55Z

pkg/util/discardedseries/tracker.go

+		t.Unlock()
+	}
+
+	seriesCounter, ok := userCounter.userSeriesMap[user]


We need a lock for the user series map

yeya24 · 2025-08-29T23:26:03Z

pkg/util/discardedseries/tracker.go

+		userCounter.Unlock()
+	}
+
+	if _, ok := seriesCounter.seriesCountMap[series]; !ok {


yeya24 · 2025-08-29T23:26:51Z

pkg/util/discardedseries/tracker.go

+
+func (t *DiscardedSeriesTracker) UpdateMetrics() {
+	usersToDelete := make([]string, 0)
+	for label, userCounter := range t.labelUserMap {


We need read lock here

yeya24 · 2025-08-29T23:33:53Z

pkg/util/discardedseries/tracker.go

+type UserCounter struct {
+	*sync.RWMutex
+	userSeriesMap map[string]*SeriesCounter
+}


Let's try to not expose those structs if we don't need access from other packages. Unless we have a good reason to mark them as public

eeldaly · 2025-09-11T17:43:52Z

Will recommit my changes since the pr now includes files I did not change when I went back to sign old commits

Signed-off-by: Essam Eldaly <[email protected]>

pull-request-size bot added the size/L label Aug 28, 2025

dosubot bot added the component/ingester label Aug 28, 2025

yeya24 reviewed Aug 28, 2025

View reviewed changes

danielblando reviewed Aug 28, 2025

View reviewed changes

yeya24 reviewed Aug 29, 2025

View reviewed changes

eeldaly force-pushed the master branch from 1f909aa to e6da572 Compare September 11, 2025 17:10

pull-request-size bot added size/XL and removed size/L labels Sep 11, 2025

eeldaly closed this Sep 11, 2025

eeldaly force-pushed the master branch from e6da572 to 45b52d3 Compare September 11, 2025 17:44

pull-request-size bot added size/XS and removed size/XL labels Sep 11, 2025

Add metric for number of series with discarded samples

fc7d10b

Signed-off-by: Essam Eldaly <[email protected]>

eeldaly reopened this Sep 11, 2025

pull-request-size bot added size/L and removed size/XS labels Sep 11, 2025

eeldaly added 2 commits September 11, 2025 13:31

Add cleanup for metric

757d225

Signed-off-by: Essam Eldaly <[email protected]>

Update changelog

4670c02

Signed-off-by: Essam Eldaly <[email protected]>

eeldaly requested review from danielblando and yeya24 September 12, 2025 17:11

	const (
	sampleOutOfOrder = "sample-out-of-order"
	newValueForTimestamp = "new-value-for-timestamp"
	sampleOutOfBounds = "sample-out-of-bounds"
	sampleTooOld = "sample-too-old"
	nativeHistogramSample = "native-histogram-sample"
	)

	const (
	perUserSeriesLimit = "per_user_series_limit"
	perUserNativeHistogramSeriesLimit = "per_user_native_histogram_series_limit"
	perMetricSeriesLimit = "per_metric_series_limit"
	perLabelsetSeriesLimit = "per_labelset_series_limit"
	)

Add metric for number of series with discarded samples #6996

Are you sure you want to change the base?

Add metric for number of series with discarded samples #6996

Uh oh!

Conversation

eeldaly commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SungJin1212 Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eeldaly commented Sep 11, 2025

Uh oh!

Uh oh!

eeldaly commented Aug 28, 2025 •

edited

Loading

SungJin1212 Aug 29, 2025 •

edited

Loading