-
Notifications
You must be signed in to change notification settings - Fork 411
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore(internal): move rate limiter to PyO3 (#9232)
This PR sets up a new `ddtrace.internal._core` module which is a Rust module defined in `src/core/` and uses PyO3. The first component being moved over is the `ddtrace.internal.rate_limiter.RateLimter`. This is a well isolated component which has minimal need to be in Python. There are clear performance gains from moving this component to native. The main motivation from this change is to start to build the basis for adding/moving performance critical components to PyO3. ## Risks This introduces a requirement on the rust compiler for building from source. We have built this into our dev image for awhile, but there are other places where we do not yet have the proper setup and so processes may fail. For example, the benchmarking platform image does not have rust compiler yet. ## Testing Since we kept the same public interface as the original Python module, we are using the existing Python tests as the validation of this change. They should be comprehensive enough to validate the new native version. ## Benchmarks The benchmarking image is not yet updated to include rust compiler, so the following results are from running locally on my machine only. The new `rate_limiter` benchmark shows a roughly 3x performance improvement. | Benchmark | main | rate_limiter | |-----------------------------|------|--------------| | ratelimiter-defaults | 801 ns | 224 ns: 3.57x faster | | ratelimiter-no_rate_limit | 325 ns | 223 ns: 1.46x faster | | ratelimiter-low_rate_limit | 800 ns | 220 ns: 3.64x faster | | ratelimiter-high_rate_limit | 817 ns | 224 ns: 3.65x faster | | ratelimiter-short_window | 928 ns | 224 ns: 4.14x faster | | ratelimiter-long_window | 808 ns | 220 ns: 3.68x faster | | Geometric mean | (ref) | 3.19x faster | This is a great improvement, however, the rate limiter is such a small portion of an actual trace, because right now it's biggest impact is on starting a new trace and making a sampling decision. So applications with the biggest possible improvement are those which start a lot of small traces at high volume. Benchmarks for the `span` suite show a minor improvement, this is because the rate limiter today only takes up a small portion of the total lifecycle of a span. | Benchmark | main | rate_limiter | |------------------------------|------|--------------| | span-add-metrics | 39.2 ms | 38.3 ms: 1.02x faster | | span-start-finish | 16.6 ms | 15.9 ms: 1.05x faster | | span-start-finish-traceid128 | 17.8 ms | 16.5 ms: 1.08x faster | | Geometric mean | (ref) | 1.02x faster | Benchmarks for the `tracer` suite showing similar results to `span`. | Benchmark | main | rate_limiter | |------------------------------|------|--------------| | tracer-small | 94.0 us | 91.2 us: 1.03x faster | | tracer-medium | 844 us | 818 us: 1.03x faster | | Geometric mean | (ref) | 1.02x faster | `tracer-large` results are not show a statistically significant enough difference in overhead. Benchmarks for the `flask_simple` suite showing almost no improvement at all. | Benchmark | main | rate_limiter | |------------------------------|------|--------------| | flasksimple-tracer | 1.13 ms | 1.15 ms: 1.02x slower | | flasksimple-debugger | 566 us | 573 us: 1.01x slower | | flasksimple-appsec-post | 1.03 ms | 1.05 ms: 1.02x slower | | flasksimple-appsec-telemetry | 1.15 ms | 1.17 ms: 1.01x slower | | Geometric mean | (ref) | 1.01x slower | ## Follow-up future work - Move the RateLimiter tests over to rust - Add Rust formatting, static analysis, testing steps to the CI process ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. ## Reviewer Checklist - [ ] Title is accurate - [ ] All changes are related to the pull request's stated goal - [ ] Description motivates each change - [ ] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [ ] Testing strategy adequately addresses listed risks - [ ] Change is maintainable (easy to change, telemetry, documentation) - [ ] Release note makes sense to a user of the library - [ ] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [ ] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- Loading branch information
1 parent
5f5c395
commit dec6694
Showing
17 changed files
with
675 additions
and
159 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
name: "Rust CI" | ||
on: | ||
push: | ||
pull_request: | ||
paths: | ||
- src/** | ||
|
||
jobs: | ||
check: | ||
name: Rust CI | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
extension: ["src/core"] | ||
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Install latest stable toolchain and rustfmt | ||
run: rustup update stable && rustup default stable && rustup component add rustfmt clippy | ||
- name: Run cargo build | ||
run: cargo build | ||
working-directory: ${{ matrix.extension }} | ||
- name: Run cargo fmt | ||
run: cargo fmt --all -- --check | ||
working-directory: ${{ matrix.extension }} | ||
- name: Run cargo clippy | ||
run: cargo clippy -- -D warnings | ||
working-directory: ${{ matrix.extension }} | ||
- name: Run cargo test | ||
run: cargo test --no-fail-fast --locked | ||
working-directory: ${{ matrix.extension }} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
defaults: &defaults | ||
rate_limit: 100 | ||
time_window: 1000000000 | ||
num_windows: 100 | ||
no_rate_limit: | ||
<<: *defaults | ||
rate_limit: 0 | ||
low_rate_limit: | ||
<<: *defaults | ||
rate_limit: 1 | ||
high_rate_limit: | ||
<<: *defaults | ||
rate_limit: 10000 | ||
short_window: | ||
<<: *defaults | ||
time_window: 100000 | ||
long_window: | ||
<<: *defaults | ||
time_window: 1000000000000 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import math | ||
|
||
import bm | ||
|
||
|
||
class RateLimiter(bm.Scenario): | ||
rate_limit = bm.var(type=int) | ||
time_window = bm.var(type=int) | ||
num_windows = bm.var(type=int) | ||
|
||
def run(self): | ||
from ddtrace.internal.compat import time_ns | ||
from ddtrace.internal.rate_limiter import RateLimiter | ||
|
||
rate_limiter = RateLimiter(rate_limit=self.rate_limit, time_window=self.time_window) | ||
|
||
def _(loops): | ||
# Divide the operations into self.num_windows time windows | ||
# DEV: We want to exercise the rate limiter across multiple windows, and we | ||
# want to ensure we get consistency in the number of windows we are using | ||
start = time_ns() | ||
windows = [start + (i * self.time_window) for i in range(self.num_windows)] | ||
per_window = math.floor(loops / self.num_windows) | ||
|
||
for window in windows: | ||
for _ in range(per_window): | ||
rate_limiter.is_allowed(window) | ||
|
||
yield _ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import os | ||
import subprocess | ||
|
||
import bm | ||
|
||
|
||
class Startup(bm.Scenario): | ||
ddtrace_run = bm.var_bool() | ||
import_ddtrace = bm.var_bool() | ||
import_ddtrace_auto = bm.var_bool() | ||
env = bm.var(type=dict) | ||
|
||
def run(self): | ||
env = os.environ.copy() | ||
env.update(self.env) | ||
|
||
args = ["python", "-c", ""] | ||
if self.import_ddtrace: | ||
args = ["python", "-c", "import ddtrace"] | ||
elif self.import_ddtrace_auto: | ||
args = ["python", "-c", "import ddtrace.auto"] | ||
elif self.ddtrace_run: | ||
args = ["ddtrace-run", "python", "-c", ""] | ||
|
||
def _(loops): | ||
for _ in range(loops): | ||
subprocess.check_call(args, env=env) | ||
|
||
yield _ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import typing | ||
|
||
class RateLimiter: | ||
""" | ||
A token bucket rate limiter implementation | ||
""" | ||
|
||
rate_limit: int | ||
time_window: float | ||
effective_rate: float | ||
current_window_rate: float | ||
prev_window_rate: typing.Optional[float] | ||
tokens: float | ||
max_tokens: float | ||
tokens_allowed: int | ||
tokens_total: int | ||
last_update_ns: float | ||
current_window_ns: float | ||
|
||
def __init__(self, rate_limit: int, time_window: float = 1e9): | ||
""" | ||
Constructor for RateLimiter | ||
:param rate_limit: The rate limit to apply for number of requests per second. | ||
rate limit > 0 max number of requests to allow per second, | ||
rate limit == 0 to disallow all requests, | ||
rate limit < 0 to allow all requests | ||
:type rate_limit: :obj:`int` | ||
:param time_window: The time window where the rate limit applies in nanoseconds. default value is 1 second. | ||
:type time_window: :obj:`float` | ||
""" | ||
def is_allowed(self, timestamp_ns: int) -> bool: | ||
""" | ||
Check whether the current request is allowed or not | ||
This method will also reduce the number of available tokens by 1 | ||
:param int timestamp_ns: timestamp in nanoseconds for the current request. | ||
:returns: Whether the current request is allowed or not | ||
:rtype: :obj:`bool` | ||
""" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.