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
• [x] Documentation added
• [x] 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]

@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

[tesla59]

@friedrichg should i remove the field from the config struct itself? https://github.com/cortexproject/cortex/blob/057313a8d4d26702ed6252468e71a0abfa96ab8b/pkg/distributor/distributor.go#L141

[CharlieTLe]

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

https://github.com/cortexproject/cortex/blob/057313a8d4d26702ed6252468e71a0abfa96ab8b/pkg/distributor/distributor.go#L141

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? https://github.com/cortexproject/cortex/blob/057313a8d4d26702ed6252468e71a0abfa96ab8b/pkg/distributor/distributor.go#L141

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?

https://github.com/cortexproject/cortex/blob/057313a8d4d26702ed6252468e71a0abfa96ab8b/pkg/distributor/distributor.go#L141

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: https://github.com/cortexproject/cortex/blob/498635f2b8e385d85fbfcdfa8127e26a4918ac89/cmd/cortex/main.go#L245-L248

[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

[stale[bot]]

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.