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

XRay Remote Sampler - Cap quotaBalance to 1x quota #3652

Merged
merged 12 commits into from May 25, 2023

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Mar 24, 2023

Tracking issue: #3651

Issue:

  • When XRay Sampling quota is run out, quotaBalance is refreshed.
  • When quotaBalance is refreshed, it is increased by: (seconds since last refresh) * quota
    • If quotaBalance is refreshed 5 seconds after previous refresh, then quotaBalance gains 5*quota. This is not desirable as it can allow for an unexpected increase burst of sampled requests after longer time periods between requests.

FIx:
Cap rule quotaBalance to 1x quota.

@jj22ee jj22ee requested review from a team and Aneurysm9 as code owners March 24, 2023 22:02
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #3652 (0fb1dd1) into main (0be2d5b) will not change coverage.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3652   +/-   ##
=====================================
  Coverage   79.3%   79.3%           
=====================================
  Files        165     165           
  Lines      10422   10422           
=====================================
  Hits        8274    8274           
  Misses      2012    2012           
  Partials     136     136           
Impacted Files Coverage Δ
samplers/aws/xray/internal/reservoir.go 100.0% <100.0%> (ø)

@jj22ee
Copy link
Contributor Author

jj22ee commented Apr 13, 2023

@Aneurysm9 Pinging for review!

@jj22ee
Copy link
Contributor Author

jj22ee commented May 2, 2023

@Aneurysm9
Can this be merged?

@Aneurysm9
Copy link
Member

@Aneurysm9 Can this be merged?

This requires at least one more approval from @open-telemetry/go-approvers before it can be merged.

@jj22ee
Copy link
Contributor Author

jj22ee commented May 4, 2023

@Aneurysm9

Does this require go-approvers to be in the Reviewers list? Can this be done or how can we ping another go-approver to look at this PR?

@jj22ee
Copy link
Contributor Author

jj22ee commented May 12, 2023

@Aneurysm9 @MrAlias
Pinging for a merge!

CHANGELOG.md Show resolved Hide resolved
@MrAlias MrAlias merged commit ad26ab3 into open-telemetry:main May 25, 2023
22 checks passed
MrAlias added a commit that referenced this pull request May 26, 2023
Move change to unreleased section.
@MrAlias MrAlias added this to the v0.43.0 milestone Aug 28, 2023
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

4 participants