Deprecate -distributor.shard-by-all-labels

[tesla59]

What this PR does:
Remove the flag distributor.shard-by-all-labels and sets it to true by default

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

Checklist

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

[tesla59]

@friedrichg is there any additional step required to fix this? https://github.com/cortexproject/cortex/actions/runs/9547290567/job/26311997676?pr=6022#step:9:12

[friedrichg]

Thank you! only nits now

[friedrichg]

@tesla59 the lint needs to pass too
An error occurred while generating the doc: config=github.com/cortexproject/cortex/pkg/distributor.Config: unable to find CLI flag for 'ShardByAllLabels' config entry exit status 1

[friedrichg]

Sorry, I did another pass and these are still related and should be removed:

grep -R ShardByAllLabels *
pkg/cortex/cortex.go:	if err := c.LimitsConfig.Validate(c.Distributor.ShardByAllLabels); err != nil {
pkg/cortex/modules.go:	t.Cfg.Ingester.DistributorShardByAllLabels = t.Cfg.Distributor.ShardByAllLabels
pkg/ingester/ingester.go:	DistributorShardByAllLabels bool   `yaml:"-"`
pkg/ingester/ingester.go:		cfg.DistributorShardByAllLabels,
pkg/ingester/ingester.go:		cfg.DistributorShardByAllLabels,
pkg/distributor/query.go:	if !d.cfg.ShardByAllLabels && len(matchers) > 0 {
pkg/distributor/distributor.go:	ShardByAllLabels         bool   `yaml:"shard_by_all_labels"`
pkg/distributor/distributor.go:	cfg.ShardByAllLabels = true
pkg/distributor/distributor.go:	if d.cfg.ShardByAllLabels {
pkg/distributor/distributor.go:	if d.cfg.ShardByAllLabels {
pkg/distributor/distributor_test.go:			distributorCfg.ShardByAllLabels = true
pkg/distributor/distributor_test.go:		distributorCfg.ShardByAllLabels = cfg.shardByAllLabels
pkg/distributor/distributor_test.go:func TestShardByAllLabelsReturnsWrongResultsForUnsortedLabels(t *testing.T) {

just assume the value is true and organize the code accordingly

[tesla59]

@friedrichg should i remove the field from the config struct itself?

cortex/pkg/distributor/distributor.go

Line 141 in 057313a

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

[CharlieTLe]

@friedrichg should i remove the field from the config struct itself?

cortex/pkg/distributor/distributor.go

Line 141 in 057313a

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

If this is removed, wouldn't this cause problems for config files that still reference this field?

[tesla59]

@friedrichg should i remove the field from the config struct itself?

cortex/pkg/distributor/distributor.go

Line 141 in 057313a

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

If this is removed, wouldn't this cause problems for config files that still reference this field?

Can we assume that the value is true and remove wherever its referenced?

[CharlieTLe]

@friedrichg should i remove the field from the config struct itself?

cortex/pkg/distributor/distributor.go

Line 141 in 057313a

ShardByAllLabels bool `yaml:"shard_by_all_labels"`

If this is removed, wouldn't this cause problems for config files that still reference this field?

Can we assume that the value is true and remove wherever its referenced?

I think the flag and field in the config struct should still exist, but marked as deprecated until it's fully removed. Perhaps the default should be set to true, and if someone is still setting it to false, then an error or warning should appear letting them know that the behavior is changing.

If we completely remove the field, people that are upgrading to this version will see an error like:

 error loading config from /config/cortex-config.yaml: Error parsing config file: yaml: unmarshal errors:
   line 21: field shard_by_all_labels not found in type distributor.Config

More info about the unmarshalling:

cortex/cmd/cortex/main.go

Lines 245 to 248 in 498635f

	err = yaml.UnmarshalStrict(buf, cfg)
	if err != nil {
	return errors.Wrap(err, "Error parsing config file")
	}

[friedrichg]

pkg/ingester/limiter_test.go:228:167: too many arguments in call to NewLimiter
	have (*validation.Overrides, *ringCountMock, string, bool, int, bool, string)
	want (*validation.Overrides, RingCount, string, int, bool, string)

the test should compile @tesla59. A review is not worth for code that doesn't compile, I feel.

[tesla59]

Hey @friedrichg
Sorry for being inactive. I was busy with some work so couldn't get to it. I believe this needs some more discussion on how to proceed further

Also I'm not sure about how u got notification for review. I didn't ping u because I was still working on fixing the build

[friedrichg]

@tesla59 Just trying to guide the PR on the next step to follow. Thanks for your work so far!

[danielblando]

We are in the process of releasing 1.18
Please rebase PR and change the changelog to master