NH-124861: redo token calculation #238
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors the token bucket implementation to use a time-based calculation approach instead of a timer-thread model. The main change eliminates the background thread that periodically replenished tokens, replacing it with on-demand calculations based on elapsed time since last usage. This improves thread safety and simplifies the implementation.
Key Changes:
- Replaced timer-based token replenishment with time-elapsed calculations
- Changed rate parameter from "tokens per interval" to "tokens per second"
- Added proper thread safety with mutex locks for all token bucket operations
- Deprecated
sample_rateandsampling_rateconfiguration options
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/solarwinds_apm/sampling/token_bucket.rb | Complete refactoring to use time-based token calculation, removed timer thread, added mutex locking for thread safety |
| lib/solarwinds_apm/sampling/oboe_sampler.rb | Removed timer start calls and reorganized mutex synchronization in update_settings and get_settings methods |
| lib/solarwinds_apm/config.rb | Deprecated sample_rate and sampling_rate configuration options with simplified warning messages |
| test/sampling/token_bucket_test.rb | Updated tests to reflect new token bucket behavior with time-based replenishment and added thread safety tests |
| test/sampling/oboe_sampler_test.rb | Changed bucket initialization from TokenBucket objects to hash-based settings |
| test/Dockerfile | Updated Ruby version from 3.1.0 to 3.2.6 and removed SWIG installation |
| @settings = {} | ||
| return | ||
| @settings_mutex.synchronize do | ||
| return if @settings.empty? |
There was a problem hiding this comment.
I was looking with copilot help on the question of early returns from a mutex synchronized block, whether that would release the lock. Seems yes (at least for MRI) but also seems break could be better. It also raised the question that the early return here, and later if settings expired in line 333, means the return value for get_settings could be nil and the caller needs to properly handle that--does it?
There was a problem hiding this comment.
Yes, they will return nil and be handled by unless sample_state.settings
There was a problem hiding this comment.
hmm but i see before that line there's also https://github.com/solarwinds/apm-ruby/blob/NH-124861/lib/solarwinds_apm/sampling/oboe_sampler.rb#L77
if sample_state.headers['X-Trace-Options']
which would throw an error if sample_state is nil?
There was a problem hiding this comment.
I don't think sample_state will be nil. It will always create the struct from SampleState.new after entering should_sample?
There was a problem hiding this comment.
OMG i can't read, yes you're right. But now also seeing https://github.com/solarwinds/apm-ruby/blob/7.1.0/lib/solarwinds_apm/sampling/oboe_sampler.rb#L133
elsif sample_state.settings[:flags].anybits?(Flags::SAMPLE_START)
which would access a possibly nill settings? guess my point is, is there some way to ensure the sample_state.settings is a hash such that callers don't have to worry about it?
cheempz
left a comment
cheempz
left a comment
There was a problem hiding this comment.
Thanks for the revisit, one remaining question @xuan-cao-swi
| @settings = {} | ||
| return | ||
| @settings_mutex.synchronize do | ||
| return if @settings.empty? |
There was a problem hiding this comment.
hmm but i see before that line there's also https://github.com/solarwinds/apm-ruby/blob/NH-124861/lib/solarwinds_apm/sampling/oboe_sampler.rb#L77
if sample_state.headers['X-Trace-Options']
which would throw an error if sample_state is nil?
Description
Test (if applicable)