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 - Preserve previous rule reservoir if rule property has not changed #3620

Merged
merged 10 commits into from May 2, 2023

Conversation

jj22ee
Copy link
Contributor

@jj22ee jj22ee commented Mar 23, 2023

Tracking issue: #3619
Issue summary: When Sampling Rules are updated, the previous rule reservoir is removed, which will take away any sampling quota from the user. When Rules are updated very frequently, the reservoir is removed frequently. The previous rule should be preserved if the rule property hasn't changed.

FIx:
When updating Rules, if the new Rule.ruleProperties hasn't changed, then preserve the previous Rule (which will also preserve the Rule reservoir and Rule samplingStatistics).

@jj22ee jj22ee requested review from a team and Aneurysm9 as code owners March 23, 2023 05:48
CHANGELOG.md Outdated Show resolved Hide resolved
@dmathieu
Copy link
Member

Could you add tests?

@jj22ee jj22ee requested review from dmathieu and removed request for willarmiros March 23, 2023 22:07
Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

lgtm!

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Merging #3620 (a582122) into main (8538a77) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3620   +/-   ##
=====================================
  Coverage   70.4%   70.5%           
=====================================
  Files        149     149           
  Lines       7123    7133   +10     
=====================================
+ Hits        5020    5030   +10     
  Misses      1972    1972           
  Partials     131     131           
Impacted Files Coverage Δ
samplers/aws/xray/internal/manifest.go 90.5% <100.0%> (+0.3%) ⬆️

@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 @dmathieu
Can this be merged?

CHANGELOG.md Outdated Show resolved Hide resolved
@Aneurysm9 Aneurysm9 merged commit 46bde6d into open-telemetry:main May 2, 2023
22 checks passed
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