Skip to content

Conversation

SungJin1212
Copy link
Member

@SungJin1212 SungJin1212 commented Aug 19, 2025

The current override-exporter only exposes a limited set of fields. This PR changes the logic to expose all fields that can be converted to a float64 type.

For reviewers: Would it be better to expose only the metrics that have values different from their defaults?

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]

@dosubot dosubot bot added the type/feature label Aug 19, 2025
@SungJin1212 SungJin1212 force-pushed the Improve-overrides-exporter branch from 91aa01b to 8c3af7f Compare August 19, 2025 12:09
@SungJin1212 SungJin1212 requested a review from friedrichg August 20, 2025 01:04
@friedrichg
Copy link
Member

please take a look @bogdan-st

Copy link
Contributor

@bogdan-st bogdan-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I don't have much to say other than LGTM

@danielblando
Copy link
Contributor

For reviewers: Would it be better to expose only the metrics that have values different from their defaults?

I like to export even default so we can have data of which value is actually configured, but for another point, I am worry about number of metrics that we gonna start generating. Do we want to start exposing all this metrics? Is there a valid cases for this? In cases where we have hundreds or thousands of tenants, this can explode. For prometheus that is not a real problem

@SungJin1212
Copy link
Member Author

@danielblando
I think it's good to expose default values. But since we're exposing the cortex_overrides metric only for tenants registered in the runtime config, I'm wondering if this could cause a series explosion. WDYT?

@danielblando
Copy link
Contributor

@SungJin1212
Ah I missed that. Valid point. I think it is fine then, but it makes sense to not export default if we are just exporting override tenants. If not exported we can assume it is the default i guess

@SungJin1212 SungJin1212 force-pushed the Improve-overrides-exporter branch from 5027a1c to 6dda5f8 Compare August 22, 2025 00:53
@SungJin1212
Copy link
Member Author

@danielblando
Let's go with this for now, and if we need to expose the default later, we can do so.

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I tested this with this runtime.yaml

runtime.yaml

ingester_limits:
    max_series: 4.8e+06
ingester_stream_chunks_when_using_blocks: true
overrides:
    default:
        compactor_blocks_retention_period: "0"
        ingestion_burst_size: 200000
        ingestion_rate: 10000
        ingestion_tenant_shard_size: 3
        max_global_series_per_metric: 20000
        max_global_series_per_user: 150000
        max_series_per_metric: 0
        max_series_per_user: 0
        ruler_max_rule_groups_per_tenant: 35
        ruler_max_rules_per_rule_group: 20
Screenshot 2025-09-01 at 8 53 29 AM

Which I am ok with.

@SungJin1212 could you mention in the CHANGELOG that label
max_local_series_per_metric got renamed to max_series_per_metric
and
max_local_series_per_user got renamed to max_series_per_user
and also adjust the docs

cortex_overrides{limit_name="max_local_series_per_metric",user="user1"} 0
cortex_overrides{limit_name="max_local_series_per_user",user="user1"} 0

@SungJin1212 SungJin1212 force-pushed the Improve-overrides-exporter branch from 6dda5f8 to 3c8e003 Compare September 2, 2025 01:47
@SungJin1212
Copy link
Member Author

@friedrichg
Thanks for catching. I updated latest commit.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 2, 2025
@friedrichg friedrichg merged commit 95a097a into cortexproject:master Sep 4, 2025
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size/L type/feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants