Skip to content

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

Merged
MrAlias merged 12 commits intoopen-telemetry:mainfrom
jj22ee:cap-quotabalance
May 25, 2023
Merged

XRay Remote Sampler - Cap quotaBalance to 1x quota#3652
MrAlias merged 12 commits intoopen-telemetry:mainfrom
jj22ee:cap-quotabalance

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 a review from a team March 24, 2023 22:02
@jj22ee jj22ee requested a review from Aneurysm9 as a code owner 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!

@MrAlias MrAlias merged commit ad26ab3 into open-telemetry:main May 25, 2023
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.

4 participants