From 2bb1845fbf4bf1b8ca7d2518fe199149b76b87e2 Mon Sep 17 00:00:00 2001 From: Ying WANG Date: Thu, 26 Dec 2024 21:33:05 +0100 Subject: [PATCH] add test for discarded samples --- pkg/costattribution/manager_test.go | 8 +-- pkg/costattribution/testutils/test_utils.go | 9 ++- pkg/costattribution/tracker.go | 7 +- pkg/distributor/validate_test.go | 64 +++++++++++++------ .../activeseries/active_series_test.go | 4 +- 5 files changed, 64 insertions(+), 28 deletions(-) diff --git a/pkg/costattribution/manager_test.go b/pkg/costattribution/manager_test.go index 2acda2ffaeb..81a94a674c9 100644 --- a/pkg/costattribution/manager_test.go +++ b/pkg/costattribution/manager_test.go @@ -18,7 +18,7 @@ import ( func newTestManager() *Manager { logger := log.NewNopLogger() - limits, _ := testutils.GetMockCostAttributionLimits(0) + limits, _ := testutils.NewMockCostAttributionLimits(0) reg := prometheus.NewRegistry() manager, err := NewManager(5*time.Second, time.Second, 10*time.Second, logger, limits, reg) if err != nil { @@ -81,7 +81,7 @@ func TestManager_CreateDeleteTracker(t *testing.T) { t.Run("Disabling user cost attribution", func(t *testing.T) { var err error - manager.limits, err = testutils.GetMockCostAttributionLimits(1) + manager.limits, err = testutils.NewMockCostAttributionLimits(1) assert.NoError(t, err) assert.NoError(t, manager.purgeInactiveAttributionsUntil(time.Unix(11, 0).Unix())) assert.Equal(t, 1, len(manager.trackersByUserID)) @@ -96,7 +96,7 @@ func TestManager_CreateDeleteTracker(t *testing.T) { t.Run("Updating user cardinality and labels", func(t *testing.T) { var err error - manager.limits, err = testutils.GetMockCostAttributionLimits(2) + manager.limits, err = testutils.NewMockCostAttributionLimits(2) assert.NoError(t, err) assert.NoError(t, manager.purgeInactiveAttributionsUntil(time.Unix(12, 0).Unix())) assert.Equal(t, 1, len(manager.trackersByUserID)) @@ -147,7 +147,7 @@ func TestManager_PurgeInactiveAttributionsUntil(t *testing.T) { t.Run("Purge after inactive timeout", func(t *testing.T) { // disable cost attribution for user1 to test purging - manager.limits, _ = testutils.GetMockCostAttributionLimits(1) + manager.limits, _ = testutils.NewMockCostAttributionLimits(1) assert.NoError(t, manager.purgeInactiveAttributionsUntil(time.Unix(5, 0).Unix())) // User3's tracker should remain since it's active, user1's tracker should be removed diff --git a/pkg/costattribution/testutils/test_utils.go b/pkg/costattribution/testutils/test_utils.go index 7ca86eb9154..f79f4861cd9 100644 --- a/pkg/costattribution/testutils/test_utils.go +++ b/pkg/costattribution/testutils/test_utils.go @@ -4,7 +4,7 @@ package testutils import "github.com/grafana/mimir/pkg/util/validation" -func GetMockCostAttributionLimits(idx int) (*validation.Overrides, error) { +func NewMockCostAttributionLimits(idx int, lvs ...string) (*validation.Overrides, error) { baseLimits := map[string]*validation.Limits{ "user1": {MaxCostAttributionCardinalityPerUser: 5, CostAttributionLabels: []string{"team"}}, "user2": {MaxCostAttributionCardinalityPerUser: 2, CostAttributionLabels: []string{}}, @@ -12,7 +12,12 @@ func GetMockCostAttributionLimits(idx int) (*validation.Overrides, error) { "user4": {MaxCostAttributionCardinalityPerUser: 5, CostAttributionLabels: []string{"platform"}}, "user5": {MaxCostAttributionCardinalityPerUser: 10, CostAttributionLabels: []string{"a"}}, } - + if len(lvs) > 0 { + baseLimits[lvs[0]] = &validation.Limits{ + MaxCostAttributionCardinalityPerUser: 10, + CostAttributionLabels: lvs[1:], + } + } switch idx { case 1: baseLimits["user1"].CostAttributionLabels = []string{} diff --git a/pkg/costattribution/tracker.go b/pkg/costattribution/tracker.go index 705af9a4196..14e66b3a424 100644 --- a/pkg/costattribution/tracker.go +++ b/pkg/costattribution/tracker.go @@ -269,7 +269,12 @@ func (t *Tracker) updateObservations(key []byte, ts int64, activeSeriesIncrement } if discardedSampleIncrement > 0 && reason != nil { o.discardedSampleMtx.Lock() - o.discardedSample[*reason] = *atomic.NewFloat64(discardedSampleIncrement) + if v, ok := o.discardedSample[*reason]; ok { + v.Add(discardedSampleIncrement) + o.discardedSample[*reason] = v + } else { + o.discardedSample[*reason] = *atomic.NewFloat64(discardedSampleIncrement) + } o.discardedSampleMtx.Unlock() } } else if len(t.observed) < t.maxCardinality*2 && createIfDoesNotExist { diff --git a/pkg/distributor/validate_test.go b/pkg/distributor/validate_test.go index 9707b89f378..d0539a9d45b 100644 --- a/pkg/distributor/validate_test.go +++ b/pkg/distributor/validate_test.go @@ -14,6 +14,7 @@ import ( "time" "unicode/utf8" + "github.com/go-kit/log" "github.com/gogo/protobuf/proto" "github.com/grafana/dskit/grpcutil" "github.com/grafana/dskit/httpgrpc" @@ -25,6 +26,8 @@ import ( grpcstatus "google.golang.org/grpc/status" golangproto "google.golang.org/protobuf/proto" + "github.com/grafana/mimir/pkg/costattribution" + catestutils "github.com/grafana/mimir/pkg/costattribution/testutils" "github.com/grafana/mimir/pkg/mimirpb" "github.com/grafana/mimir/pkg/util/validation" ) @@ -75,8 +78,13 @@ func TestValidateLabels(t *testing.T) { cfg.maxLabelValueLength = 25 cfg.maxLabelNameLength = 25 - cfg.maxLabelNamesPerSeries = 2 - cfg.maxLabelNamesPerInfoSeries = 3 + cfg.maxLabelNamesPerSeries = 3 + cfg.maxLabelNamesPerInfoSeries = 4 + limits, _ := catestutils.NewMockCostAttributionLimits(0, userID, "team") + careg := prometheus.NewRegistry() + manager, err := costattribution.NewManager(5*time.Second, time.Second, 10*time.Second, log.NewNopLogger(), limits, careg) + require.NoError(t, err) + cat := manager.Tracker(userID) for _, c := range []struct { metric model.Metric @@ -85,25 +93,25 @@ func TestValidateLabels(t *testing.T) { err error }{ { - metric: map[model.LabelName]model.LabelValue{}, + metric: map[model.LabelName]model.LabelValue{"team": "a"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: errors.New(noMetricNameMsgFormat), }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: " "}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: " ", "team": "a"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf(invalidMetricNameMsgFormat, " "), }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "metric_name_with_\xb0_invalid_utf8_\xb0"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "metric_name_with_\xb0_invalid_utf8_\xb0", "team": "a"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf(invalidMetricNameMsgFormat, "metric_name_with__invalid_utf8_ (non-ascii characters removed)"), }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "valid", "foo ": "bar"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "valid", "foo ": "bar", "team": "a"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf( @@ -113,18 +121,19 @@ func TestValidateLabels(t *testing.T) { []mimirpb.LabelAdapter{ {Name: model.MetricNameLabel, Value: "valid"}, {Name: "foo ", Value: "bar"}, + {Name: "team", Value: "a"}, }, ), ), }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "valid"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "valid", "team": "c"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: nil, }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "badLabelName", "this_is_a_really_really_long_name_that_should_cause_an_error": "test_value_please_ignore"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "badLabelName", "this_is_a_really_really_long_name_that_should_cause_an_error": "test_value_please_ignore", "team": "biz"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf( @@ -132,6 +141,7 @@ func TestValidateLabels(t *testing.T) { "this_is_a_really_really_long_name_that_should_cause_an_error", mimirpb.FromLabelAdaptersToString( []mimirpb.LabelAdapter{ + {Name: "team", Value: "biz"}, {Name: model.MetricNameLabel, Value: "badLabelName"}, {Name: "this_is_a_really_really_long_name_that_should_cause_an_error", Value: "test_value_please_ignore"}, }, @@ -139,7 +149,7 @@ func TestValidateLabels(t *testing.T) { ), }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "badLabelValue", "much_shorter_name": "test_value_please_ignore_no_really_nothing_to_see_here"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "badLabelValue", "much_shorter_name": "test_value_please_ignore_no_really_nothing_to_see_here", "team": "biz"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf( @@ -150,12 +160,13 @@ func TestValidateLabels(t *testing.T) { []mimirpb.LabelAdapter{ {Name: model.MetricNameLabel, Value: "badLabelValue"}, {Name: "much_shorter_name", Value: "test_value_please_ignore_no_really_nothing_to_see_here"}, + {Name: "team", Value: "biz"}, }, ), ), }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "bar": "baz", "blip": "blop"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "bar": "baz", "blip": "blop", "team": "plof"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf( @@ -165,21 +176,22 @@ func TestValidateLabels(t *testing.T) { {Name: model.MetricNameLabel, Value: "foo"}, {Name: "bar", Value: "baz"}, {Name: "blip", Value: "blop"}, + {Name: "team", Value: "plof"}, }, - 2, + 3, )..., ), }, { // *_info metrics have higher label limits. - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo_info", "bar": "baz", "blip": "blop"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo_info", "bar": "baz", "blip": "blop", "team": "a"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: nil, }, { // *_info metrics have higher label limits. - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo_info", "bar": "baz", "blip": "blop", "blap": "blup"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo_info", "bar": "baz", "blip": "blop", "blap": "blup", "team": "a"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf( @@ -190,31 +202,32 @@ func TestValidateLabels(t *testing.T) { {Name: "bar", Value: "baz"}, {Name: "blip", Value: "blop"}, {Name: "blap", Value: "blup"}, + {Name: "team", Value: "a"}, }, - 3, + 4, )..., ), }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "bar": "baz", "blip": "blop"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "bar": "baz", "blip": "blop", "team": "a"}, skipLabelNameValidation: false, skipLabelCountValidation: true, err: nil, }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "invalid%label&name": "bar"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "invalid%label&name": "bar", "team": "biz"}, skipLabelNameValidation: true, skipLabelCountValidation: false, err: nil, }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "label1": "你好"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "label1": "你好", "team": "plof"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: nil, }, { - metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "label1": "abc\xfe\xfddef"}, + metric: map[model.LabelName]model.LabelValue{model.MetricNameLabel: "foo", "label1": "abc\xfe\xfddef", "team": "plof"}, skipLabelNameValidation: false, skipLabelCountValidation: false, err: fmt.Errorf( @@ -229,7 +242,7 @@ func TestValidateLabels(t *testing.T) { err: nil, }, } { - err := validateLabels(s, cfg, userID, "custom label", mimirpb.FromMetricsToLabelAdapters(c.metric), c.skipLabelNameValidation, c.skipLabelCountValidation, nil, ts) + err := validateLabels(s, cfg, userID, "custom label", mimirpb.FromMetricsToLabelAdapters(c.metric), c.skipLabelNameValidation, c.skipLabelCountValidation, cat, ts) assert.Equal(t, c.err, err, "wrong error") } @@ -250,6 +263,19 @@ func TestValidateLabels(t *testing.T) { cortex_discarded_samples_total{group="custom label",reason="random reason",user="different user"} 1 `), "cortex_discarded_samples_total")) + require.NoError(t, testutil.GatherAndCompare(careg, strings.NewReader(` + # HELP cortex_discarded_attributed_samples_total The total number of samples that were discarded per attribution. + # TYPE cortex_discarded_attributed_samples_total counter + cortex_discarded_attributed_samples_total{reason="label_invalid",team="a",tenant="testUser",tracker="cost-attribution"} 1 + cortex_discarded_attributed_samples_total{reason="label_name_too_long",team="biz",tenant="testUser",tracker="cost-attribution"} 1 + cortex_discarded_attributed_samples_total{reason="label_value_invalid",team="plof",tenant="testUser",tracker="cost-attribution"} 1 + cortex_discarded_attributed_samples_total{reason="label_value_too_long",team="biz",tenant="testUser",tracker="cost-attribution"} 1 + cortex_discarded_attributed_samples_total{reason="max_label_names_per_info_series",team="a",tenant="testUser",tracker="cost-attribution"} 1 + cortex_discarded_attributed_samples_total{reason="max_label_names_per_series",team="plof",tenant="testUser",tracker="cost-attribution"} 1 + cortex_discarded_attributed_samples_total{reason="metric_name_invalid",team="a",tenant="testUser",tracker="cost-attribution"} 2 + cortex_discarded_attributed_samples_total{reason="missing_metric_name",team="a",tenant="testUser",tracker="cost-attribution"} 1 +`), "cortex_discarded_attributed_samples_total")) + s.deleteUserMetrics(userID) require.NoError(t, testutil.GatherAndCompare(reg, strings.NewReader(` diff --git a/pkg/ingester/activeseries/active_series_test.go b/pkg/ingester/activeseries/active_series_test.go index a0c48e8d7fb..d4e16150b38 100644 --- a/pkg/ingester/activeseries/active_series_test.go +++ b/pkg/ingester/activeseries/active_series_test.go @@ -241,7 +241,7 @@ type mockIndex struct { existingLabels map[storage.SeriesRef]labels.Labels } -func (m *mockIndex) Series(ref storage.SeriesRef, builder *labels.ScratchBuilder, chks *[]chunks.Meta) error { +func (m *mockIndex) Series(ref storage.SeriesRef, builder *labels.ScratchBuilder, _ *[]chunks.Meta) error { if ls, ok := m.existingLabels[ref]; ok { builder.Assign(ls) return nil @@ -250,7 +250,7 @@ func (m *mockIndex) Series(ref storage.SeriesRef, builder *labels.ScratchBuilder } func TestActiveSeries_UpdateSeries_WithCostAttribution(t *testing.T) { - limits, _ := catestutils.GetMockCostAttributionLimits(0) + limits, _ := catestutils.NewMockCostAttributionLimits(0) reg := prometheus.NewRegistry() manager, err := costattribution.NewManager(5*time.Second, time.Second, 10*time.Second, log.NewNopLogger(), limits, reg) require.NoError(t, err)