[ratelimiter] Remove wrong error, remove duplicate code #578

constanca-m · 2025-05-26T10:13:05Z

This PR:

Removes the too many requests error wrongly introduced in [Ratelimit Processor] Instrument the ratelimiter service with telemetry metrics #562.
Removes the duplicate code to declare metrics

processor/ratelimitprocessor/gubernator.go

constanca-m · 2025-05-26T10:15:26Z

processor/ratelimitprocessor/gubernator.go

-		limitPercentUsed = 1.0 - min(1, float64(resp.Remaining)/float64(resp.Limit))
-	}
-
-	limitBucket := getLimitThresholdBucket(limitPercentUsed)


@gizas @kaiyan-sheng I am (temporarily) removing this - I can add it again in this PR, I removed it just to clean up. Can you give me more details on the choice of adding this? And why local.go does not have this? Maybe by mistake?

I am also having trouble interpreting this attribute value. What does it mean? I understand the limit % part, but then I get lost on telemetry.WithLimitThreshold(limitBucket),.

This is the relevant test case here:

opentelemetry-collector-components/processor/ratelimitprocessor/gubernator_test.go

Line 311 in 1c294d1

limitUsedPercent := []int64{20, 30, 60, 80, 90, 95, 100}

Seems we added a new attribute https://github.com/elastic/opentelemetry-collector-components/blob/main/processor/ratelimitprocessor/gubernator_test.go#L343C7-L343C16

@gizas I am still having trouble on understanding this attribute. Can you help me on this?

So I understand limitPercentUsed part. But then I get lost on limitBucket.

I then have two questions (apart from understanding what limitBucket means):

Why doesn't local rate limiter have this attribute as well?

What is the choice of adding this attribute?

If we want to keep this attribute, the code needs to change so RateLimit inside the local/gubernator rate limiters starts returning a response struct instead of an error. For example:

type response struct { limitBucket int err error }

I am trying to understand if this change is necessary (and how to do the same for the local rate limiter), or if we can discard this attribute for now.

I believe this is dictating the token bucket algorithm limit threshold in the guberator - Basically metric indicating what percentage of limit we are at/available? I am not sure how usable this is, feels like this has been carried over from MIS, I ll let @lahsivjar to comment if this was useful in his experience.

Trying to see the code, seems that we calculate the limitbucket and we add it as an attribute. I dont see any other use.

And yes we should add it in local.go if we decide to add it.

I will wait for @kaiyan-sheng, @lahsivjar to comment. I would say we can remove it for now

Yes we should add it in local.go if we think this metric is useful so we are reporting the same metrics for both. This is from MIS indeed. We can leave it out for now if it's not gonna be useful or unsure.

(apologies for not seeing this thread, my git notification management skills are on the decline)

https://github.com/elastic/apm-ingest-service/blob/baaf48072f5be1df0ca5e6dbaef351b96233ae0a/internal/ratelimit/ratelimit.go#L90-L98

I am not very familiar with the rate limiter code, but reviewing the comments in the above code, it appears that the purpose of limitBucket is to give histogram-like features that can be plotted as a dimension with fixed cardinality.

While not that familiar with the code, I have used the dashboard that is powered by the above dimension, and it is useful to be able to see all the projects with >= than, say 75%, throttling limit. So, I would be in favor of keeping the limitBucket in the metric as a dimension. For consistency, we can also add it to local rate limiter.

Thank you @lahsivjar! I will add limitBucket back and into local rate limiter in a separate pr.

gizas · 2025-05-26T13:35:07Z

processor/ratelimitprocessor/gubernator_test.go

 	assert.Equal(t, "ratelimit/abc123", req.Requests[0].Name)
 	assert.Equal(t, "metadata_key:value2", req.Requests[0].UniqueKey)
 }

-// Ensures that specific metrics are returned in case of relevant responses of GetRateLimits
-func TestGubernatorRateLimiter_RateLimit_Meterprovider(t *testing.T) {


Why u deleted this test case? I had added in order to check the different scenarios and the attributes :). Is it covered above in TestGubernatorRateLimiter_RateLimit ?

The TestRateLimitThresholdAttribute is not tested for sure

The tests for the telemetry metrics were moved to processor_test.go, since the metrics are now only changed there. There is a test on traces, profiles, metrics and logs now

The TestRateLimitThresholdAttribute is not tested for sure

Yes, this test is not there at the moment. I am trying to make sense of this attribute and how we can put it in both local and gubernator rate limiter.

Now that I am more awake 😆 Here is the link for the tests:

opentelemetry-collector-components/processor/ratelimitprocessor/processor_test.go

Line 269 in 4356def

func testRateLimitRequests(t *testing.T, tt testTelemetry) {

And you can see that we check the telemetry for every rate limit function. For example, metrics rate limit:

opentelemetry-collector-components/processor/ratelimitprocessor/processor_test.go

Line 188 in 4356def

testRateLimitRequests(t, tt)

processor/ratelimitprocessor/processor.go

constanca-m · 2025-05-27T20:08:23Z

processor/ratelimitprocessor/gubernator_test.go

@@ -204,183 +183,16 @@ func TestGubernatorRateLimiter_RateLimit_MetadataKeys(t *testing.T) {

 	// Each unique combination of metadata keys should get its own rate limit.
 	err = rateLimiter.RateLimit(clientContext1, 1)
-	assert.EqualError(t, err, "too many requests")
+	assert.NoError(t, err)


All these test changes come from the error introduced in PR #562 (throwing an error after the rate limiter has waited). I put the unit test back as it was since the beginning until that PR got merged.

vigneshshanmugam

Few nits, but Looks good otherwise.

We also need to remove the limit threshold from metadata.yaml

opentelemetry-collector-components/processor/ratelimitprocessor/metadata.yaml

Line 29 in 4356def

limit_threshold:

if we agree to get the attribute removed.

processor/ratelimitprocessor/processor.go

processor/ratelimitprocessor/gubernator.go

vigneshshanmugam · 2025-05-27T20:23:15Z

processor/ratelimitprocessor/gubernator.go

-		limitPercentUsed = 1.0 - min(1, float64(resp.Remaining)/float64(resp.Limit))
-	}
-
-	limitBucket := getLimitThresholdBucket(limitPercentUsed)


I believe this is dictating the token bucket algorithm limit threshold in the guberator - Basically metric indicating what percentage of limit we are at/available? I am not sure how usable this is, feels like this has been carried over from MIS, I ll let @lahsivjar to comment if this was useful in his experience.

processor/ratelimitprocessor/gubernator_test.go

vigneshshanmugam · 2025-05-27T20:32:15Z

processor/ratelimitprocessor/processor_test.go

+	testRateLimitRequests(t, tt)
+}
+
+func testRateLimitRequests(t *testing.T, tt testTelemetry) {


Not a solid blocker, But we can make use of the metadatatest for these instead of creating manualreader, provider etc.

telemetry := componenttest.NewTelemetry() tb, err := metadata.NewTelemetryBuilder(telemetry.NewTelemetrySettings()) require.NoError(t, err) defer tb.Shutdown() // construct expected attributes based on the metrics name and invoke mdatatest expectedAttrs := attribute.NewSet(attrs...) metadatatest.AssertEqualRatelimitRequests(t, tt, []metricdata.DataPoint[int64]{ { Value: 2, // tune this Attributes: expectedAttrs, }, }, metricdatatest.IgnoreTimestamp())

I love this suggestion, thank you! I wasn't aware this existed.

vigneshshanmugam

LGTM, I ll defer to others for adding limit_threshold if its deemed useful. Thanks

remove wrong error, remove duplicate code

4f09a82

constanca-m requested a review from a team as a code owner May 26, 2025 10:13

constanca-m requested review from girodav and stevejgordon May 26, 2025 10:13

constanca-m commented May 26, 2025

View reviewed changes

processor/ratelimitprocessor/gubernator.go Show resolved Hide resolved

constanca-m commented May 26, 2025

View reviewed changes

constanca-m requested review from gizas, vigneshshanmugam and kaiyan-sheng May 26, 2025 10:16

constanca-m added 4 commits May 26, 2025 12:16

make gogenerate && make license-update

befae7c

fix unit test

c72ddc5

fix unit test

6558ce6

make gogenerate && make license-update

246f9ce

gizas reviewed May 26, 2025

View reviewed changes

vigneshshanmugam reviewed May 26, 2025

View reviewed changes

processor/ratelimitprocessor/processor.go Outdated Show resolved Hide resolved

clean up

4356def

constanca-m commented May 26, 2025

View reviewed changes

processor/ratelimitprocessor/processor.go Show resolved Hide resolved

constanca-m commented May 27, 2025

View reviewed changes

vigneshshanmugam reviewed May 27, 2025

View reviewed changes

constanca-m added 2 commits May 28, 2025 09:38

remove extra attribute, use metadatatest

0d8e7a1

use default on switch statement

f7b5ef6

vigneshshanmugam approved these changes May 28, 2025

View reviewed changes

constanca-m merged commit 66579a5 into elastic:main May 29, 2025
13 checks passed

constanca-m deleted the ratelimiter-spawn-gubernator branch May 29, 2025 04:13

[ratelimiter] Remove wrong error, remove duplicate code #578

[ratelimiter] Remove wrong error, remove duplicate code #578

Uh oh!

Conversation

constanca-m commented May 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constanca-m May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahsivjar May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constanca-m May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constanca-m May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constanca-m May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

constanca-m May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vigneshshanmugam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vigneshshanmugam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

constanca-m May 26, 2025 •

edited

Loading

lahsivjar May 29, 2025 •

edited

Loading

constanca-m May 26, 2025 •

edited

Loading

constanca-m May 26, 2025 •

edited

Loading

constanca-m May 26, 2025 •

edited

Loading

constanca-m May 27, 2025 •

edited

Loading