Skip to content

Commit 4eeed54

Browse files
authored
NH-124861: redo token calculation (#238)
* NH-124861: redo token calculation
1 parent d9e6dab commit 4eeed54

File tree

10 files changed

+161
-183
lines changed

10 files changed

+161
-183
lines changed

.github/instructions/ruby.instructions.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ end
4949
- Use snake_case for method names: `set_transaction_name`, `should_sample?`, `parent_based_algo`
5050
- Use `?` suffix for predicate methods that return boolean: `ready?`, `valid?`, `running?`, `boolean?`
5151
- Use `!` suffix sparingly for destructive methods or methods with side effects
52-
- Use `=` suffix for setter methods: `capacity=`, `tokens=`, `interval=`
52+
- Use `=` suffix for setter methods: `capacity=`, `tokens=`
5353

5454
### Variables
5555
- Use snake_case: `sample_state`, `trace_flags`, `parent_span`, `service_name`
@@ -58,7 +58,7 @@ end
5858
- Class variables: `@@config` (use sparingly, prefer class instance variables)
5959

6060
### Constants
61-
- Use SCREAMING_SNAKE_CASE: `SAMPLE_RATE_ATTRIBUTE`, `MAX_INTERVAL`, `OTEL_SAMPLING_DECISION`
61+
- Use SCREAMING_SNAKE_CASE: `SAMPLE_RATE_ATTRIBUTE`, `OTEL_SAMPLING_DECISION`
6262
- Freeze constant collections: `SW_LOG_LEVEL_MAPPING.freeze`, `EXEC_ISH_METHODS.freeze`
6363
- Group related constants at the top of the class or module
6464

@@ -280,7 +280,7 @@ end
280280

281281
- Use `Struct.new` for simple data containers:
282282
```ruby
283-
TokenBucketSettings = Struct.new(:capacity, :rate, :interval, :type)
283+
TokenBucketSettings = Struct.new(:capacity, :rate, :type)
284284
```
285285

286286
- Add methods to Struct subclasses when needed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ rubocop_result.txt
5050
git_push.sh
5151
test_run.sh
5252
test.js
53+
review.txt
5354

5455
# act.secrets
5556
act.secrets

lib/solarwinds_apm/config.rb

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -192,29 +192,14 @@ def self.[]=(key, value)
192192
case key
193193
when :sampling_rate
194194
SolarWindsAPM.logger.warn do
195-
"[#{name}/#{__method__}] sampling_rate is not a supported setting for SolarWindsAPM::Config. Please use :sample_rate."
195+
'[Deprecated] sampling_rate is not a supported setting for SolarWindsAPM::Config.'
196196
end
197197

198198
when :sample_rate
199-
unless value.is_a?(Integer) || value.is_a?(Float)
200-
SolarWindsAPM.logger.warn do
201-
"[#{name}/#{__method__}] :sample_rate must be a number between 0 and 1000000 (1m) (provided: #{value}), corrected to 0"
202-
end
203-
value = 0
204-
end
205-
206-
# Validate :sample_rate value
207-
unless value.between?(0, 1e6)
208-
new_value = value.negative? ? 0 : 1_000_000
209-
SolarWindsAPM.logger.warn do
210-
"[#{name}/#{__method__}] :sample_rate must be between 0 and 1000000 (1m) (provided: #{value}), corrected to #{new_value}"
211-
end
199+
SolarWindsAPM.logger.warn do
200+
'[Deprecated] sample_rate is not a supported setting for SolarWindsAPM::Config.'
212201
end
213202

214-
# Assure value is an integer
215-
@@config[key.to_sym] = new_value.to_i
216-
SolarWindsAPM.sample_rate(new_value)
217-
218203
when :transaction_settings
219204
compile_settings(value)
220205

lib/solarwinds_apm/sampling/oboe_sampler.rb

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ class OboeSampler
1818
TRIGGERED_TRACE_ATTRIBUTE = 'TriggeredTrace'
1919

2020
TRACESTATE_REGEXP = /^[0-9a-f]{16}-[0-9a-f]{2}$/
21-
BUCKET_INTERVAL = 1000
2221
DICE_SCALE = 1_000_000
2322

2423
OTEL_SAMPLING_DECISION = ::OpenTelemetry::SDK::Trace::Samplers::Decision
@@ -30,16 +29,14 @@ def initialize(logger)
3029
@counters = SolarWindsAPM::Metrics::Counter.new
3130
@buckets = {
3231
SolarWindsAPM::BucketType::DEFAULT =>
33-
SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(nil, nil, BUCKET_INTERVAL, 'DEFUALT')),
32+
SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(nil, nil, 'DEFUALT')),
3433
SolarWindsAPM::BucketType::TRIGGER_RELAXED =>
35-
SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(nil, nil, BUCKET_INTERVAL, 'TRIGGER_RELAXED')),
34+
SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(nil, nil, 'TRIGGER_RELAXED')),
3635
SolarWindsAPM::BucketType::TRIGGER_STRICT =>
37-
SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(nil, nil, BUCKET_INTERVAL, 'TRIGGER_STRICT'))
36+
SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(nil, nil, 'TRIGGER_STRICT'))
3837
}
3938
@settings = {} # parsed setting from swo backend
4039
@settings_mutex = ::Mutex.new
41-
42-
@buckets.each_value(&:start)
4340
end
4441

4542
# return sampling result
@@ -289,9 +286,9 @@ def disabled_algo(sample_state)
289286
end
290287

291288
def update_settings(settings)
292-
return unless settings[:timestamp] > (@settings[:timestamp] || 0)
293-
294289
@settings_mutex.synchronize do
290+
return unless settings[:timestamp] > (@settings[:timestamp] || 0)
291+
295292
@settings = settings
296293
@buckets.each do |type, bucket|
297294
bucket.update(@settings[:buckets][type]) if @settings[:buckets][type]
@@ -324,18 +321,20 @@ def sw_from_span_and_decision(parent_span, otel_decision)
324321
end
325322

326323
def get_settings(params)
327-
return if @settings.empty?
328-
329-
expiry = (@settings[:timestamp] + @settings[:ttl]) * 1000
330-
time_now = Time.now.to_i * 1000
331-
if time_now > expiry
332-
@logger.debug { "[#{self.class}/#{__method__}] settings expired, removing" }
333-
@settings = {}
334-
return
324+
@settings_mutex.synchronize do
325+
return if @settings.empty?
326+
327+
expiry = (@settings[:timestamp] + @settings[:ttl]) * 1000
328+
time_now = Time.now.to_i * 1000
329+
if time_now > expiry
330+
@logger.debug { "[#{self.class}/#{__method__}] settings expired, removing" }
331+
@settings = {}
332+
return
333+
end
334+
sampling_setting = SolarWindsAPM::SamplingSettings.merge(@settings, local_settings(params))
335+
@logger.debug { "[#{self.class}/#{__method__}] sampling_setting: #{sampling_setting.inspect}" }
336+
sampling_setting
335337
end
336-
sampling_setting = SolarWindsAPM::SamplingSettings.merge(@settings, local_settings(params))
337-
@logger.debug { "[#{self.class}/#{__method__}] sampling_setting: #{sampling_setting.inspect}" }
338-
sampling_setting
339338
end
340339
end
341340
end

lib/solarwinds_apm/sampling/sampling_constants.rb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ module TriggerTrace
5555

5656
TokenBucketSettings = Struct.new(:capacity, # Number
5757
:rate, # Number
58-
:interval, # Number
5958
:type) # String
6059

6160
module SampleSource

lib/solarwinds_apm/sampling/token_bucket.rb

Lines changed: 56 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -10,126 +10,88 @@
1010
# capacity is updated through update_settings
1111
module SolarWindsAPM
1212
class TokenBucket
13-
# Maximum value of a signed 32-bit integer
14-
MAX_INTERVAL = (2**31) - 1
15-
16-
attr_reader :capacity, :rate, :interval, :tokens, :type
13+
attr_reader :type
1714

1815
def initialize(token_bucket_settings)
19-
self.capacity = token_bucket_settings.capacity || 0
20-
self.rate = token_bucket_settings.rate || 0
21-
self.interval = token_bucket_settings.interval || MAX_INTERVAL
22-
self.tokens = @capacity
16+
@capacity = token_bucket_settings.capacity || 0
17+
@rate = token_bucket_settings.rate || 0
18+
@tokens = @capacity
19+
@last_update_time = Time.now.to_f
2320
@type = token_bucket_settings.type
24-
@timer = nil
21+
@lock = ::Mutex.new
2522
end
2623

27-
# oboe sampler update_settings will update the token
28-
# (thread safe as update_settings is guarded by mutex from oboe sampler)
29-
def update(settings)
30-
settings.instance_of?(Hash) ? update_from_hash(settings) : update_from_token_bucket_settings(settings)
24+
def capacity
25+
@lock.synchronize { @capacity }
3126
end
3227

33-
def update_from_hash(settings)
34-
if settings[:capacity]
35-
difference = settings[:capacity] - @capacity
36-
self.capacity = settings[:capacity]
37-
self.tokens = @tokens + difference
38-
end
39-
40-
self.rate = settings[:rate] if settings[:rate]
41-
42-
return unless settings[:interval]
43-
44-
self.interval = settings[:interval]
45-
return unless running
46-
47-
stop
48-
start
28+
def rate
29+
@lock.synchronize { @rate }
4930
end
5031

51-
def update_from_token_bucket_settings(settings)
52-
if settings.capacity
53-
difference = settings.capacity - @capacity
54-
self.capacity = settings.capacity
55-
self.tokens = @tokens + difference
32+
def tokens
33+
@lock.synchronize do
34+
calculate_tokens
35+
@tokens
5636
end
57-
58-
self.rate = settings.rate if settings.rate
59-
60-
return unless settings.interval
61-
62-
self.interval = settings.interval
63-
return unless running
64-
65-
stop
66-
start
67-
end
68-
69-
def capacity=(capacity)
70-
@capacity = [0, capacity].max
71-
end
72-
73-
def rate=(rate)
74-
@rate = [0, rate].max
75-
end
76-
77-
# self.interval= sets the @interval and @sleep_interval
78-
# @sleep_interval is used in the timer thread to sleep between replenishing the bucket
79-
def interval=(interval)
80-
@interval = interval.clamp(0, MAX_INTERVAL)
81-
@sleep_interval = @interval / 1000.0
8237
end
8338

84-
def tokens=(tokens)
85-
@tokens = tokens.clamp(0, @capacity)
39+
# oboe sampler update_settings will update the token
40+
def update(settings)
41+
settings.instance_of?(Hash) ? update_from_hash(settings) : update_from_hash(tb_to_hash(settings))
8642
end
8743

8844
# Attempts to consume tokens from the bucket
89-
# @param n [Integer] Number of tokens to consume
45+
# @param token [Integer] Number of tokens to consume
9046
# @return [Boolean] Whether there were enough tokens
91-
# TODO: we need to include thread-safety here since sampler is shared across threads
92-
# and we may have multiple threads trying to consume tokens at the same time
9347
def consume(token = 1)
94-
if @tokens >= token
95-
self.tokens = @tokens - token
96-
SolarWindsAPM.logger.debug { "[#{self.class}/#{__method__}] #{@type} Consumed #{token} from total #{@tokens} (#{(@tokens.to_f / @capacity * 100).round(1)}% remaining)" }
97-
true
98-
else
99-
SolarWindsAPM.logger.debug { "[#{self.class}/#{__method__}] #{@type} Token consumption failed: requested=#{token}, available=#{@tokens}, capacity=#{@capacity}" }
100-
false
101-
end
102-
end
103-
104-
# Starts replenishing the bucket
105-
def start
106-
return if running
107-
108-
@timer = Thread.new do
109-
loop do
110-
task
111-
sleep(@sleep_interval)
48+
@lock.synchronize do
49+
calculate_tokens
50+
if @tokens >= token
51+
@tokens -= token
52+
SolarWindsAPM.logger.debug { "[#{self.class}/#{__method__}] #{@type} Consumed #{token} (#{(@tokens.to_f / @capacity * 100).round(1)}% remaining)" }
53+
true
54+
else
55+
SolarWindsAPM.logger.debug { "[#{self.class}/#{__method__}] #{@type} Token consumption failed: requested=#{token}, available=#{@tokens}, capacity=#{@capacity}" }
56+
false
11257
end
11358
end
11459
end
11560

116-
# Stops replenishing the bucket
117-
def stop
118-
return unless running
61+
private
11962

120-
@timer.kill
121-
@timer = nil
63+
def calculate_tokens
64+
now = Time.now.to_f
65+
elapsed = now - @last_update_time
66+
@last_update_time = now
67+
@tokens += elapsed * @rate
68+
@tokens = [@tokens, @capacity].min
12269
end
12370

124-
# Whether the bucket is actively being replenished
125-
def running
126-
!@timer.nil?
127-
end
71+
# settings is from json sampler
72+
def update_from_hash(settings)
73+
@lock.synchronize do
74+
calculate_tokens
75+
76+
if settings[:capacity]
77+
new_capacity = [0, settings[:capacity]].max
78+
difference = new_capacity - @capacity
79+
@capacity = new_capacity
80+
@tokens += difference
81+
@tokens = [0, @tokens].max
82+
end
12883

129-
private
84+
@rate = [0, settings[:rate]].max if settings[:rate]
85+
end
86+
end
13087

131-
def task
132-
self.tokens = tokens + @rate
88+
# settings is from http sampler
89+
def tb_to_hash(settings)
90+
tb_hash = {}
91+
tb_hash[:capacity] = settings.capacity if settings.respond_to?(:capacity)
92+
tb_hash[:rate] = settings.rate if settings.respond_to?(:rate)
93+
tb_hash[:type] = settings.type if settings.respond_to?(:type)
94+
tb_hash
13395
end
13496
end
13597
end

test/Dockerfile

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -60,19 +60,10 @@ RUN echo 'alias be="bundle exec"' >> ~/.profile
6060
# install rubies to build our gem against
6161
RUN . ~/.profile \
6262
&& cd /root/.rbenv/plugins/ruby-build && git pull && cd - \
63-
&& rbenv install 3.1.0
63+
&& rbenv install 3.2.6
6464

6565
RUN echo 'gem: --no-document' >> ~/.gemrc
6666

67-
# install swig 4.0.2
68-
RUN curl -SL https://github.com/swig/swig/archive/refs/tags/v4.0.2.tar.gz \
69-
| tar xzC /tmp \
70-
&& cd /tmp/swig-4.0.2 \
71-
&& ./autogen.sh \
72-
&& ./configure && make && make install \
73-
&& cd - \
74-
&& rm -rf /tmp/swig-4.0.2
75-
7667
# set up github package credentials for pushing
7768
RUN mkdir ~/.gem \
7869
&& echo "---\n:github: Bearer ${BUNDLE_RUBYGEMS__PKG__GITHUB__COM}" >> ~/.gem/credentials \

test/sampling/oboe_sampler_test.rb

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -425,8 +425,8 @@
425425
sample_source: SolarWindsAPM::SampleSource::LOCAL_DEFAULT,
426426
flags: SolarWindsAPM::Flags::SAMPLE_START | SolarWindsAPM::Flags::TRIGGERED_TRACE,
427427
buckets: {
428-
SolarWindsAPM::BucketType::TRIGGER_STRICT => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(10, 5, BUCKET_INTERVAL)),
429-
SolarWindsAPM::BucketType::TRIGGER_RELAXED => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(0, 0, BUCKET_INTERVAL))
428+
SolarWindsAPM::BucketType::TRIGGER_STRICT => { capacity: 10, rate: 5 },
429+
SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 0, rate: 0 }
430430
},
431431
timestamp: Time.now.to_i,
432432
ttl: 10
@@ -464,8 +464,8 @@
464464
sample_source: SolarWindsAPM::SampleSource::LOCAL_DEFAULT,
465465
flags: SolarWindsAPM::Flags::SAMPLE_START | SolarWindsAPM::Flags::TRIGGERED_TRACE,
466466
buckets: {
467-
SolarWindsAPM::BucketType::TRIGGER_STRICT => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(0, 0, BUCKET_INTERVAL)),
468-
SolarWindsAPM::BucketType::TRIGGER_RELAXED => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(20, 10, BUCKET_INTERVAL))
467+
SolarWindsAPM::BucketType::TRIGGER_STRICT => { capacity: 0, rate: 0 },
468+
SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 20, rate: 10 }
469469
},
470470
timestamp: Time.now.to_i,
471471
ttl: 10
@@ -502,8 +502,8 @@
502502
sample_source: SolarWindsAPM::SampleSource::LOCAL_DEFAULT,
503503
flags: SolarWindsAPM::Flags::SAMPLE_START | SolarWindsAPM::Flags::TRIGGERED_TRACE,
504504
buckets: {
505-
SolarWindsAPM::BucketType::TRIGGER_STRICT => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(0, 0, BUCKET_INTERVAL)),
506-
SolarWindsAPM::BucketType::TRIGGER_RELAXED => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(20, 10, BUCKET_INTERVAL))
505+
SolarWindsAPM::BucketType::TRIGGER_STRICT => { capacity: 0, rate: 0 },
506+
SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 20, rate: 10 }
507507
},
508508
signature_key: 'key',
509509
timestamp: Time.now.to_i,
@@ -546,8 +546,8 @@
546546
sample_source: SolarWindsAPM::SampleSource::LOCAL_DEFAULT,
547547
flags: SolarWindsAPM::Flags::SAMPLE_START | SolarWindsAPM::Flags::TRIGGERED_TRACE,
548548
buckets: {
549-
SolarWindsAPM::BucketType::TRIGGER_STRICT => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(10, 5, BUCKET_INTERVAL)),
550-
SolarWindsAPM::BucketType::TRIGGER_RELAXED => SolarWindsAPM::TokenBucket.new(SolarWindsAPM::TokenBucketSettings.new(0, 0, BUCKET_INTERVAL))
549+
SolarWindsAPM::BucketType::TRIGGER_STRICT => { capacity: 10, rate: 5 },
550+
SolarWindsAPM::BucketType::TRIGGER_RELAXED => { capacity: 0, rate: 0 }
551551
},
552552
signature_key: 'key',
553553
timestamp: Time.now.to_i,

0 commit comments

Comments
 (0)