search

jaegertracing / jaeger-lib

A collection of shared infrastructure libraries used by different components of Jaeger.
https://github.com/uber/jaeger
Apache License 2.0
67 stars 34 forks source link

Improve rate limiter API #53

Closed isaachier closed 6 years ago

isaachier commented 6 years ago

Which problem is this PR solving?

Short description of the changes

isaachier commented 6 years ago

@yurishkuro the reason this is in jaeger-lib was to migrate it from jaeger-client-go. But @black-adder had said that using utils.RateLimiter from jaeger-lib instead of jaeger-client-go would be a potential issue for backwards-compatibility (he might remember details, I'm not sure at the moment), which is why utils.RateLimiter has not been removed from the client.

Regarding the reuse of this rate limiter, I believe it should be done once well, instead of ad-hoc in each library. One API change was discussed and ultimately dismissed in #38. For that reason, the throttler in https://github.com/jaegertracing/jaeger/pull/1079 uses a custom token bucket implementation. Not adding the wait time API here would mean implementing yet another custom token bucket implementation.

isaachier commented 6 years ago

Build fix depends on #56.

yurishkuro commented 6 years ago

It is much easier to make internal type public than vice versa. You are currently trying to solve exactly one use case, which does not meet the "rule of three" condition for being public type. Also, as Prithvi suggested, why can we not just use uber-go rate limiter, which already sleeps? Let's solve problems by writing less code, not more.

isaachier commented 6 years ago

I didn't realize uber-go was an option (adding dependencies being frowned upon). Either way, I'm happy. Lol about "rule of three" (never heard outside of C++). But I think it does solve three unique cases:

All three cases need the simple token bucket implementation, so I don't see why it is not just done full scale here.

Bottom line: what do you suggest?

yurishkuro commented 6 years ago

I suggest either using uber-go limiter or an internal type. While we do have three use cases, they all have slightly different requirements and developing a single solution with a written-in-stone API is something I personally want to avoid. If anything, we can try to combine throttler with this one, but still as internal type where we don't have to worry about public API.

isaachier commented 6 years ago

Got it. Thanks!

isaachier commented 6 years ago

BTW uber-go/ratelimit only takes an integer credits per second.