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.

Sign up for GitHub

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

Jump to bottom

Adaptive sampling should preserve accumulated rate limiter balances #1729

Closed
1 of 6 tasks
yurishkuro opened this issue Aug 13, 2019 · 1 comment
Closed
1 of 6 tasks

Adaptive sampling should preserve accumulated rate limiter balances #1729

yurishkuro opened this issue Aug 13, 2019 · 1 comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement meta-issue An tracking issue that requires work in other repos

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Aug 13, 2019 •
edited
Loading

Requirement - what kind of business use case are you trying to solve?

Dynamically update lower-bound rate limiter settings in adaptive sampling.

Problem - what in Jaeger blocks you from solving the requirement?

Many Jaeger clients implement this by completely replacing rate limiters with a new object, discarding the current accumulated balance of the existing rate limiter.

Proposal - what do you suggest to solve the problem or improve the existing situation?

  • Rate limiting samplers should implement an update() method that preserves the accumulated balance.
  • Adaptive sampler should use update() method instead of replacing the rate limiting samplers.
  • Upon initialization the rate limiting samplers should apply jitter to the initial balance.

The Node.js client already implements these behaviors and can be used as a reference.

  • Go
  • Java
  • Node
  • Python
  • C#
  • C++

Related issue: #1728
@yurishkuro yurishkuro added client-libs good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement meta-issue An tracking issue that requires work in other repos labels Aug 13, 2019
@yurishkuro yurishkuro mentioned this issue Aug 13, 2019
Closed
6 tasks
@Falco20019 Falco20019 mentioned this issue Apr 16, 2020
Closed
william-tran pushed a commit to autonomic-ai/jaeger-client-java that referenced this issue Oct 14, 2021
Remove synchronized on PerOperationSampler.sample
c0cf6fd 
- Use a ConcurrentHashMap for operationNameToSampler so that any number
of concurrent calls to sample can be made and safely modify it.
- Add volatile to any fields that can be changed by the update method to
ensure visibility of changes to other threads.
- Retain instances of GuaranteedThroughputSampler to preserve their rate
limit balances across updates when parameters don't change improving on
jaegertracing/jaeger#1729

See jaegertracing#609 for
similar work.

Fixes jaegertracing#807

Signed-off-by: Will Tran <[email protected]>
@william-tran william-tran mentioned this issue Oct 14, 2021
Closed
@yurishkuro
Copy link
Member Author

Per #3362, we're sunsetting Jaeger clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement meta-issue An tracking issue that requires work in other repos
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jpkrohling @yurishkuro