Replace EnableNativeHistograms from TSDB config with PerTenant Limit #6718

PaurushGarg · 2025-04-23T13:13:38Z

Replace EnableNativeHistograms from TSDB config with EnableNativeHistogramPerUser Limit

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

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

…nableNativeHistogramPerUser limit Signed-off-by: Paurush Garg <[email protected]>

harry671003 · 2025-04-23T14:17:40Z

pkg/storage/tsdb/config.go

@@ -204,7 +201,6 @@ func (cfg *TSDBConfig) RegisterFlags(f *flag.FlagSet) {
 	f.IntVar(&cfg.MaxExemplars, "blocks-storage.tsdb.max-exemplars", 0, "Deprecated, use maxExemplars in limits instead. If the MaxExemplars value in limits is set to zero, cortex will fallback on this value. This setting enables support for exemplars in TSDB and sets the maximum number that will be stored. 0 or less means disabled.")
 	f.BoolVar(&cfg.MemorySnapshotOnShutdown, "blocks-storage.tsdb.memory-snapshot-on-shutdown", false, "True to enable snapshotting of in-memory TSDB data on disk when shutting down.")
 	f.Int64Var(&cfg.OutOfOrderCapMax, "blocks-storage.tsdb.out-of-order-cap-max", tsdb.DefaultOutOfOrderCapMax, "[EXPERIMENTAL] Configures the maximum number of samples per chunk that can be out-of-order.")
-	f.BoolVar(&cfg.EnableNativeHistograms, "blocks-storage.tsdb.enable-native-histograms", false, "[EXPERIMENTAL] True to enable native histogram.")


This will be a breaking change. But since this feature is experimental can we remove it and simplify the configurations?

@yeya24 @alanprot what do you guys think?

Is it still a breaking change if we keep the configuration name the same but move it to per tenant runtime config? I guess we don't have to rename it just to add _per_user suffix

That way existing users won't be impacted as it is allowed to be set globally

Did you mean keep the same CLI flag?

Yeah. It is still kind of breaking as it moves from tsdb to limit section in the config file. But I guess it is fine for an experimental feature as you said.

Thanks. Updated to keep the same config name.

harry671003 · 2025-04-23T14:18:24Z

pkg/util/validation/limits.go

@@ -257,6 +258,7 @@ func (l *Limits) RegisterFlags(f *flag.FlagSet) {
 	f.IntVar(&l.MaxLocalSeriesPerMetric, "ingester.max-series-per-metric", 50000, "The maximum number of active series per metric name, per ingester. 0 to disable.")
 	f.IntVar(&l.MaxGlobalSeriesPerUser, "ingester.max-global-series-per-user", 0, "The maximum number of active series per user, across the cluster before replication. 0 to disable. Supported only if -distributor.shard-by-all-labels is true.")
 	f.IntVar(&l.MaxGlobalSeriesPerMetric, "ingester.max-global-series-per-metric", 0, "The maximum number of active series per metric name, across the cluster before replication. 0 to disable.")
+	f.BoolVar(&l.EnableNativeHistogramPerUser, "ingester.enable_native_histogram_per_user", false, "Flag to enable NativeHistograms per user.")


Can you add the Experimental tag?

Thanks. Added experimental tag.

harry671003 · 2025-04-23T14:20:07Z

pkg/ingester/ingester.go

-		EnableNativeHistograms:         i.cfg.BlocksStorageConfig.TSDB.EnableNativeHistograms,
+		EnableOOONativeHistograms:      true,
+		EnableOverlappingCompaction:    false, // Always let compactors handle overlapped blocks, e.g. OOO blocks.
+		EnableNativeHistograms:         true,  // Always enable Native Histograms


Maybe explain here that the gate keeping is done though a per tenant config at ingestion?

Thanks. Added.

harry671003 · 2025-04-23T14:21:10Z

pkg/ingester/ingester.go

@@ -1347,7 +1347,7 @@ func (i *Ingester) Push(ctx context.Context, req *cortexpb.WriteRequest) (*corte
 			return nil, wrapWithUser(err, userID)
 		}

-		if i.cfg.BlocksStorageConfig.TSDB.EnableNativeHistograms {
+		if i.limits.EnableNativeHistogramPerUser(userID) {


I think we need to throw an exception here similar to Prometheus? Wdyt?

https://github.com/prometheus/prometheus/blob/main/tsdb/head_append.go#L647

Try ingesting some native histogram sample into current version of cortex with the NH feature disabled and see what is the behaviour.

I think current Cortex behaviour is that it count it as discarded sample here and not throw error here. But I m still checking by running Cortex.

Signed-off-by: Paurush Garg <[email protected]>

…ng config name Signed-off-by: Paurush Garg <[email protected]>

friedrichg

thanks!

harry671003

LGTM!

yeya24 · 2025-04-29T00:25:46Z

pkg/ingester/ingester_test.go

@@ -6839,7 +6850,7 @@ func TestIngester_UpdateLabelSetMetrics(t *testing.T) {
 	require.NoError(t, os.Mkdir(chunksDir, os.ModePerm))
 	require.NoError(t, os.Mkdir(blocksDir, os.ModePerm))

-	i, err := prepareIngesterWithBlocksStorageAndLimits(t, cfg, limits, tenantLimits, blocksDir, reg, false)
+	i, err := prepareIngesterWithBlocksStorageAndLimits(t, cfg, limits, tenantLimits, blocksDir, reg)


Do we have a test case for when native histogram is disabled and push should increment the samples discarded metrics?
If not, can you add one to TestIngester_Push?

https://github.com/cortexproject/cortex/blob/master/pkg/ingester/ingester_test.go#L1034

yeya24

Thanks. This is awesome!

Removing EnableNativeHistograms from TSDB config and replacing with E…

775b073

…nableNativeHistogramPerUser limit Signed-off-by: Paurush Garg <[email protected]>

pull-request-size bot added the size/M label Apr 23, 2025

harry671003 reviewed Apr 23, 2025

View reviewed changes

Adding experimental flag

c6b2e6a

Signed-off-by: Paurush Garg <[email protected]>

PaurushGarg requested a review from harry671003 April 24, 2025 12:04

PaurushGarg added 2 commits April 24, 2025 06:38

Adding changelog entry and make doc

f5f203e

Signed-off-by: Paurush Garg <[email protected]>

Updating to keep the EnableNativeHistograms limit name same as existi…

a022a96

…ng config name Signed-off-by: Paurush Garg <[email protected]>

PaurushGarg requested a review from yeya24 April 25, 2025 14:01

PaurushGarg changed the title ~~Replace EnableNativeHistograms from TSDB config with EnableNativeHistogramPerUser Limit~~ Replace EnableNativeHistograms from TSDB config with PerTenant Limit Apr 25, 2025

PaurushGarg marked this pull request as ready for review April 25, 2025 14:30

dosubot bot added the storage/blocks Blocks storage engine label Apr 25, 2025

friedrichg approved these changes Apr 28, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 28, 2025

harry671003 approved these changes Apr 29, 2025

View reviewed changes

yeya24 reviewed Apr 29, 2025

View reviewed changes

yeya24 approved these changes Apr 30, 2025

View reviewed changes

yeya24 merged commit 39761ee into cortexproject:master Apr 30, 2025
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace EnableNativeHistograms from TSDB config with PerTenant Limit #6718

Replace EnableNativeHistograms from TSDB config with PerTenant Limit #6718

PaurushGarg commented Apr 23, 2025 •

edited

Loading

harry671003 Apr 23, 2025

yeya24 Apr 23, 2025

harry671003 Apr 23, 2025

yeya24 Apr 23, 2025

PaurushGarg Apr 25, 2025

harry671003 Apr 23, 2025

PaurushGarg Apr 24, 2025 •

edited

Loading

harry671003 Apr 23, 2025

PaurushGarg Apr 24, 2025

harry671003 Apr 23, 2025

PaurushGarg Apr 24, 2025 •

edited

Loading

friedrichg left a comment

harry671003 left a comment

yeya24 Apr 29, 2025

PaurushGarg Apr 30, 2025

yeya24 left a comment

Replace EnableNativeHistograms from TSDB config with PerTenant Limit #6718

Replace EnableNativeHistograms from TSDB config with PerTenant Limit #6718

Conversation

PaurushGarg commented Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaurushGarg Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaurushGarg Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

friedrichg left a comment

Choose a reason for hiding this comment

harry671003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

PaurushGarg commented Apr 23, 2025 •

edited

Loading

PaurushGarg Apr 24, 2025 •

edited

Loading

PaurushGarg Apr 24, 2025 •

edited

Loading