Add active_series_additional_custom_trackers config option #10428
Conversation
|
💻 Deploy preview deleted. |
pkg/util/validation/limits.go
Outdated
| ) | ||
|
|
||
| // Cache it. | ||
| o.getOverridesForUser(userID).activeSeriesMergedCustomTrackersConfig = &merged |
There was a problem hiding this comment.
This looks racy 🤔 Shouldn't we coordinate the access here and above where we're reading it?
There was a problem hiding this comment.
You're right, it's racy and I missed it. I pushed an option to fix the race and added an high-level unit test (TestRuntimeConfigLoader_ActiveSeriesCustomTrackersMergingShouldNotInterfereBetweenTenants) to gain better confidence. I also added a unit test for the race, that fails with the previous code (when running with -race).
| @@ -3368,6 +3368,19 @@ The `limits` block configures default and per-tenant limits imposed by component | |||
| # CLI flag: -ingester.active-series-custom-trackers | |||
| [active_series_custom_trackers: <map of tracker name (string) to matcher (string)> | default = ] | |||
|
|
|||
| # (advanced) Additional custom trackers for active metrics merged on top of the | |||
| # custom trackers. This configuration option can be useful to define the base | |||
There was a problem hiding this comment.
Does merged on top of the custom trackers refer to preexisting trackers? If so, we should specify this for clarity.
There was a problem hiding this comment.
Reading the rest of this, it looks like that are merged on top of base custom trackers? I think we should say that.
There was a problem hiding this comment.
Yes your understanding is correct. I've addressed it in b66faea
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
prototype-additional-custom-trackers
branch
from
187d2a7 to
877b9ab
Compare
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
…NotInterfereBetweenTenants Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
pkg/util/validation/limits.go
Outdated
| @@ -402,6 +407,11 @@ func (l *Limits) RegisterFlags(f *flag.FlagSet) { | |||
| // Ingest storage. | |||
| f.StringVar(&l.IngestStorageReadConsistency, "ingest-storage.read-consistency", api.ReadConsistencyEventual, fmt.Sprintf("The default consistency level to enforce for queries when using the ingest storage. Supports values: %s.", strings.Join(api.ReadConsistencies, ", "))) | |||
| f.IntVar(&l.IngestionPartitionsTenantShardSize, "ingest-storage.ingestion-partition-tenant-shard-size", 0, "The number of partitions a tenant's data should be sharded to when using the ingest storage. Tenants are sharded across partitions using shuffle-sharding. 0 disables shuffle sharding and tenant is sharded across all partitions.") | |||
|
|
|||
| // Ensure the pointer holder is initialized. | |||
| if l.activeSeriesMergedCustomTrackersConfig == nil { | |||
There was a problem hiding this comment.
The fact that we have to check this here is confusing, but I'll trust that you added this because you saw the need 🤔
There was a problem hiding this comment.
I think it's not necessary. I removed it (after manual testing the removal).
Signed-off-by: Marco Pracucci <[email protected]>
What this PR does
At Grafana Labs we do an extensive usage of
active_series_custom_trackersand we've configured many of them.Typically, we define
active_series_custom_trackersfor all tenants using CLI flags. In few cases, we have per-tenant overrides. When we have per-tenant overrides, we re-defineactive_series_custom_trackersusing YAML and the per-tenant config is typically the default trackers + few additional ones that are tenant specific.This means that the per-tenant config of
active_series_custom_trackersis essentially a copy-paste of the default ones + few extra ones. This bloat the YAML config file with a bunch of trackers that are actually just the default ones.In this PR I propose to add another config option called
active_series_additional_custom_trackers. The idea is that we can useactive_series_additional_custom_trackersto configure the few extra per-tenant trackers only, and we keep having the default ones only configured throughactive_series_custom_trackers.Manual tests
I've tested it locally and looks working. In the screenshot below you can see a local Mimir cluster where I've configured the "base" trackers first, and then I've modified the runtime config to add the "additional" trackers too. The gap in the metrics is during the
cortex_ingester_active_series_loading(I've configured a 1m idle timeout for active series, to quickly test it):Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.