Return rate limit error according to ingestion rate strategy

[ie-pham]

What this PR does: Currently, for all rate_limited errors regardless of the ingestion rate strategy, Tempo returns the local strategy limit. This causes confusion for setups that use the global strategy. This PR correctly returns the limit according to the ingestion rate strategy.

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]

[ie-pham]

When running main on my local, I noticed that overrides.IngestionRateStrategy() would sometimes return defaults overrides but would also return specific tenant overrides (which doesn't have ingestion rate strategy)

When we fetch the overrides here I believe because the overrides hadn’t start up yet this function would return the defaults override which does have the ingestion rate strategy.

But then we call the same function here the overrides is up and running, using “” here actually returns a tenant specific overrides which does not contain the ingestion rate strategy.

@kvrhdn could you please double check my logic

[mdisibio]

@ie-pham What do you think about including both the local and global rate limits in the error message? My concern is the cases where a tenant exceeds the local limit, but overall traffic is still below the global limit. Catching and explaining these can be tricky. The current error message with the local limit makes it more apparent, and also captures the number of distributor replicas at the time (indirectly).

[ie-pham]

It seems the IngestionRateStrategy has always been a defaults overrides. It isn't part of the distributor config at all. Not sure if we want to spend time adding it or leave it as is.

[yvrhdn]

It seems the IngestionRateStrategy has always been a defaults overrides. It isn't part of the distributor config at all. Not sure if we want to spend time adding it or leave it as is.

I would be in favour of adding this setting to distributor config and mark the override as deprecated. It's currently really not working as you would expect from other runtime overrides.

Issues:

• only the value in default overrides is read, while this is valid it won't do what you expect:

# overrides configmap
overrides:
  "*":
    ingestion:
      rate_strategy: global
  "my-tenant":
    ingestion:
      rate_strategy: global
    # ...

• normally values in the runtime overrides can change at any time and Tempo will adjust accordingly, but we create the limiter during startup and then don't replace it anymore.

Deprecating the override imo would mean:

• introduce a setting to distributor config block
• read both values during startup but let overrides take priority
• update docs and mark the override as deprecated
• (Minor) we could log a warning during startup if the override is set (Config.CheckConfig)

And then at some point in the future we just remove the override completely.

[yvrhdn]

LGTM, this will be a nice usability improvement!

Regarding the config change, if we want to do it might be better to do it in a separate PR to not block this one.