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

Update effective_rate calculation #1236

Merged
merged 7 commits into from
Nov 13, 2020
Merged

Update effective_rate calculation #1236

merged 7 commits into from
Nov 13, 2020

Conversation

ericmustin
Copy link
Contributor

Summary

This PR Updates the Effective Rate calculation of the Rule Sampler. According to Spec, the effective rate should be the average of the last 2 time windows, each time window 1s each.

Notes

I mainly tried to mimic the dd-trace-py https://github.com/DataDog/dd-trace-py/blob/8c450b56ba87cd8cab916bb579a01837c35b0c01/ddtrace/internal/rate_limiter.py#L7 approach. We have some existing tests that mock a >1s time period and they all still pass, but if there's conditions we should test here I can try to add them.

@ericmustin ericmustin requested a review from a team November 6, 2020 09:50
@ericmustin ericmustin changed the title Fix rate limiting Update effective_rate calculation Nov 6, 2020
@brettlangdon
Copy link
Member

Python has to update since they have it slightly wrong.

We want to change the formula from:

rate = (previous_rate + current_rate) / 2

to

rate  = (previous_window_sampled_traces + current_window_sampled_traces) / (previous_window_total_traces + current_window_total_traces)

It'll give us more consistently accurate values over the two windows.

brettlangdon
brettlangdon previously approved these changes Nov 12, 2020
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we need to adjust any tests for this?

@ericmustin
Copy link
Contributor Author

ericmustin commented Nov 12, 2020

@brettlangdon Updated the specs to have tests more specific to the behavior we've added, as our previous tests were a bit too broad to ensure we can prevent a regression here.

@ericmustin ericmustin merged commit d8d265a into master Nov 13, 2020
@ericmustin ericmustin added this to the 0.43.0 milestone Nov 13, 2020
@ivoanjo ivoanjo deleted the fix_rate_limiting branch July 16, 2021 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants