Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ddcommon] Add token bucket rate limiter implementation #552

Closed
wants to merge 3 commits into from

Conversation

brettlangdon
Copy link
Member

What does this PR do?

Add an implementation of a token bucket rate limiter to ddcommon.

Motivation

Meant as a reference implementation of the rate limiter intended to be used by tracing clients for their sampling implementation.

Additional Notes

Anything else we should know when reviewing?

How to test the change?

Describe here in detail how the change can be validated.

@brettlangdon brettlangdon requested review from a team as code owners July 27, 2024 14:11
@pr-commenter
Copy link

pr-commenter bot commented Jul 27, 2024

Benchmarks

This comment was omitted because it was over 65536 characters.Please check the Gitlab Job logs to see its output.

@bwoebi
Copy link
Contributor

bwoebi commented Jul 27, 2024

Hm. A couple notes:

  • the time resolution on windows is ... rather milliseconds than nanoseconds. Not a fundamental problem, but it makes not much sense to have interval in anything but at least milliseconds, I'd say
  • the time is using system time, which is susceptible to ... system time changes, i.e. not monotonic.

I do already have an implementation of a rate limiter https://github.com/DataDog/libdatadog/blob/d79091ec56a5050f7e0a5053829b5ca5b12a2f71/sidecar/src/shm_limiters.rs
I hadn't needed it outside of sidecar yet ... but probably should have pushed it to ddcommon.

That implementation is based on atomics.
I very much need atomics as the purpose of the limiter is being able to work across processes via shared memory, something I can't just Mutex.
Also my use case involves the ability to change the rate limit on an existing limiter. (i.e. being friendly to that scenario).

Do you want to keep your implementation, or should we rather take my limiter implementation and put the LocalLimiter I have into ddcommon (and possibly the ShmLimiter into ipc crate)?

@brettlangdon
Copy link
Member Author

@bwoebi my original implementation was using clocksource as a way to avoid the issues with SystemTime, that was something I wanted to revisit here.

I don't care too much about whose implementation is taken, but having a shared token bucket rate limiter implementation in ddcommon would be good.

@brettlangdon
Copy link
Member Author

That implementation is based on atomics.
I very much need atomics as the purpose of the limiter is being able to work across processes via shared memory, something I can't just Mutex.

Is there a reason you need it to work across processes?

For most of the language libraries the sampling rate limiter is per-process.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 93.08176% with 11 lines in your changes missing coverage. Please review.

Project coverage is 70.58%. Comparing base (eb62a6d) to head (0329035).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #552      +/-   ##
==========================================
+ Coverage   70.49%   70.58%   +0.08%     
==========================================
  Files         213      214       +1     
  Lines       28412    28588     +176     
==========================================
+ Hits        20030    20179     +149     
- Misses       8382     8409      +27     
Components Coverage Δ
crashtracker 25.21% <ø> (+0.07%) ⬆️
datadog-alloc 98.73% <ø> (ø)
data-pipeline 50.00% <ø> (ø)
data-pipeline-ffi 0.00% <ø> (ø)
ddcommon 86.79% <93.08%> (+0.56%) ⬆️
ddcommon-ffi 74.58% <ø> (ø)
ddtelemetry 58.95% <ø> (ø)
ipc 84.18% <ø> (ø)
profiling 78.09% <ø> (ø)
profiling-ffi 56.76% <ø> (ø)
serverless 0.00% <ø> (ø)
sidecar 35.42% <ø> (ø)
sidecar-ffi 0.00% <ø> (ø)
spawn-worker 54.98% <ø> (ø)
trace-mini-agent 71.14% <ø> (ø)
trace-normalization 98.24% <ø> (ø)
trace-obfuscation 95.73% <ø> (ø)
trace-protobuf 77.16% <ø> (ø)
trace-utils 91.03% <ø> (-0.60%) ⬇️

@bwoebi
Copy link
Contributor

bwoebi commented Jul 29, 2024

@brettlangdon The primary problem is that PHP tends to have many processes in some configurations, with autoscaling the number of processes depending on load, making the sample rate ... unpredictable.
Which is why shared rate limiting is needed - you just set it and it's valid for your host, which is a perfectly known quantity.

@brettlangdon
Copy link
Member Author

@brettlangdon The primary problem is that PHP tends to have many processes in some configurations, with autoscaling the number of processes depending on load, making the sample rate ... unpredictable. Which is why shared rate limiting is needed - you just set it and it's valid for your host, which is a perfectly known quantity.

What is the performance overhead of using a shared memory rate limiter vs an in-process one? I'd expect it to be quite heavy?

@bwoebi
Copy link
Contributor

bwoebi commented Jul 29, 2024

@brettlangdon Why would there be any extra overhead (apart from the initial mmap call during process init)? It's just pointing to some memory.

@brettlangdon
Copy link
Member Author

@brettlangdon Why would there be any extra overhead (apart from the initial mmap call during process init)? It's just pointing to some memory.

Wouldn't we now be creating contention between processes on shared memory updates?

@bwoebi
Copy link
Contributor

bwoebi commented Jul 29, 2024

That's why the implementation is based on atomics, to avoid any lock contention.

@bwoebi
Copy link
Contributor

bwoebi commented Sep 4, 2024

Superseeded by #560.

@bwoebi bwoebi closed this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants