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

[Sampler] Allow passing in a Proc to enable/disable the sampler. #134

Merged
merged 1 commit into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -341,22 +341,29 @@ By default, the stackprof backend will be used.

The `AppProfiler` middleware can be configured to sample a percentage of requests for profiling. This can be useful for profiling a subset of requests in production without profiling every request.

To enable profile sampling:
To enable profile sampling, you can either set `profile_sampler_enabled` to `true` or provide your own custom `Proc`:

```ruby
AppProfiler.profile_sampler_enabled = true
# OR
Rails.application.config.app_profiler.profile_sampler_enabled = true
```

```ruby
AppProfiler.profile_sampler_enabled = -> { sampler_enabled? }
# OR
Rails.application.config.app_profiler.profile_sampler_enabled = -> { sampler_enabled? }
```


Both backends, StackProf and Vernier, can be configured separately.

These can be overridden like:

```ruby
AppProfiler.profile_sampler_config = AppProfiler::Sampler::Config.new(
sample_rate: 0.5,
paths: ["/foo"],
targets: ["/foo"],
backends_probability: { stackprof: 0.5, vernier: 0.5 },
backend_configs: {
stackprof: AppProfiler::Sampler::StackprofConfig.new,
Expand All @@ -367,7 +374,7 @@ AppProfiler.profile_sampler_config = AppProfiler::Sampler::Config.new(

Rails.application.config.app_profiler = AppProfiler::Sampler::Config.new(
sample_rate: 0.5,
paths: ["/foo"],
targets: ["/foo"],
backends_probability: { stackprof: 0.5, vernier: 0.5 },
backend_configs: {
stackprof: AppProfiler::Sampler::StackprofConfig.new,
Expand All @@ -380,12 +387,12 @@ All the configuration parameters are optional and have default values. The defau

```ruby

| Sampler Config | Default |
| -------- | ------- |
| sample_rate (0.0 - 1.0) | 0.001 (0.1 %) |
| paths | ['/'] |
| stackprof_probability | 1.0 |
| vernier_probability | 0.0 |
| Sampler Config | Default |
| -------- | ------- |
| sample_rate (0.0 - 1.0) | 0.001 (0.1 %) |
| targets (request paths, job_names etc ) | ['/'] |
| stackprof_probability | 1.0 |
| vernier_probability | 0.0 |

| StackprofConfig | Default |
| -------- | ------- |
Expand Down
24 changes: 23 additions & 1 deletion lib/app_profiler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ module Viewer
mattr_reader :profile_enqueue_failure, default: nil
mattr_reader :after_process_queue, default: nil
mattr_accessor :forward_metadata_on_upload, default: false
mattr_accessor :profile_sampler_enabled, default: false

mattr_accessor :profile_sampler_config

class << self
Expand Down Expand Up @@ -116,6 +116,28 @@ def backend=(new_backend)
@profiler_backend = new_profiler_backend
end

def profile_sampler_enabled=(value)
if value.is_a?(Proc)
raise ArgumentError,
"profile_sampler_enabled must be a proc or a lambda that accepts no argument" if value.arity != 0
else
raise ArgumentError, "Must be TrueClass or FalseClass" unless [TrueClass, FalseClass].include?(value.class)
end

@profile_sampler_enabled = value
end

def profile_sampler_enabled
return false unless defined?(@profile_sampler_enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return false unless defined?(@profile_sampler_enabled)

You don't need that. If it's not defined then it's nil. If it's nil it isn't a Proc. So you just return nil which is close enough to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's okay, I would like to keep it as is. We enforce True/False/Proc when setting. So if we read nil and then try to set it back, it raises - which does not seem right.


@profile_sampler_enabled.is_a?(Proc) ? @profile_sampler_enabled.call : @profile_sampler_enabled
rescue => e
logger.error(
"[AppProfiler.profile_sampler_enabled] exception: #{e}, message: #{e.message}",
)
false
bmansoob marked this conversation as resolved.
Show resolved Hide resolved
end

def backend_for(backend_name)
if vernier_supported? &&
backend_name == AppProfiler::Backend::VernierBackend.name
Expand Down
1 change: 0 additions & 1 deletion lib/app_profiler/middleware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ def profile(env, params)

def profile_params(params)
return params if params.valid?

return unless AppProfiler.profile_sampler_enabled

AppProfiler::Sampler.profile_params(params, AppProfiler.profile_sampler_config)
Expand Down
11 changes: 7 additions & 4 deletions lib/app_profiler/sampler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,21 @@ module AppProfiler
module Sampler
class << self
def profile_params(request, config)
return unless sample?(config, request)
profile_params_for(request.path, config)
end

def profile_params_for(target, config)
return unless sample?(config, target)

get_profile_params(config)
end

private

def sample?(config, request)
def sample?(config, target)
return false if Kernel.rand > config.sample_rate

path = request.path
return false unless config.paths.any? { |p| path.match?(p) }
return false unless config.targets.any? { |t| target.match?(t) }

true
end
Expand Down
13 changes: 8 additions & 5 deletions lib/app_profiler/sampler/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,31 @@
module AppProfiler
module Sampler
class Config
attr_reader :sample_rate, :paths, :cpu_interval, :backends_probability
attr_reader :sample_rate, :targets, :cpu_interval, :backends_probability

SAMPLE_RATE = 0.001 # 0.1%
PATHS = ["/"]
TARGETS = ["/"]
BACKEND_PROBABILITES = { stackprof: 1.0, vernier: 0.0 }
@backends = {}

def initialize(sample_rate: SAMPLE_RATE,
paths: PATHS,
targets: TARGETS,
backends_probability: BACKEND_PROBABILITES,
backends_config: {
stackprof: StackprofConfig.new,
})
},
paths: nil)

if sample_rate < 0.0 || sample_rate > 1.0
raise ArgumentError, "sample_rate must be between 0 and 1"
end

raise ArgumentError, "mode probabilities must sum to 1" unless backends_probability.values.sum == 1.0

ActiveSupport::Deprecation.new.warn("passing paths is deprecated, use targets instead") if paths

@sample_rate = sample_rate
@paths = paths
@targets = paths || targets
@backends_config = backends_config
@backends_probability = backends_probability
end
Expand Down
29 changes: 29 additions & 0 deletions test/app_profiler/app_profiler_configuration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,34 @@ class ConfigurationTest < TestCase
AppProfiler.after_process_queue = lambda { nil }
end
end

test "unexpected handler profile_sampler_enabled raises ArgumentError " do
assert_raises(ArgumentError) do
AppProfiler.profile_sampler_enabled = ->(arg) { arg }
end
end

test "proc with args for profile_sampler_enabled raises ArgumentError " do
assert_raises(ArgumentError) do
AppProfiler.profile_sampler_enabled = ->(arg) { arg }
end
end

test "profile_sampler_enabled allows TrueClass, FalseClass, Proc" do
old_status = AppProfiler.profile_sampler_enabled
AppProfiler.profile_sampler_enabled = false
AppProfiler.profile_sampler_enabled = true
AppProfiler.profile_sampler_enabled = -> { false }
ensure
AppProfiler.profile_sampler_enabled = old_status
end

test "profile_sampler_enabled is false if Proc raises" do
old_status = AppProfiler.profile_sampler_enabled
AppProfiler.profile_sampler_enabled = -> { raise "error" }
assert_equal(false, AppProfiler.profile_sampler_enabled)
ensure
AppProfiler.profile_sampler_enabled = old_status
end
end
end
14 changes: 13 additions & 1 deletion test/app_profiler/middleware_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ def call(env)
with_google_cloud_storage do
AppProfiler.profile_sampler_config = AppProfiler::Sampler::Config.new(
sample_rate: 1.0,
paths: ["/"],
targets: ["/"],
)

assert_profiles_dumped(0) do
Expand All @@ -399,6 +399,18 @@ def call(env)
end
end

test "request is not sampled when sampler is not enabled via Proc" do
with_google_cloud_storage do
AppProfiler.profile_sampler_config = AppProfiler::Sampler::Config.new(sample_rate: 1.0)
AppProfiler.profile_sampler_enabled = -> { false }
assert_profiles_dumped(0) do
middleware = AppProfiler::Middleware.new(app_env)
response = middleware.call(mock_request_env(path: "/"))
assert_nil(response[1]["X-Profile-Async"])
end
end
end

private

def with_profile_sampler_enabled
Expand Down
7 changes: 6 additions & 1 deletion test/app_profiler/sampler/config_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@ class ConfigTest < TestCase
test "default config" do
config = Config.new
assert_equal(Config::SAMPLE_RATE, config.sample_rate)
assert_equal(Config::PATHS, config.paths)
assert_equal(Config::TARGETS, config.targets)
assert_equal(Config::BACKEND_PROBABILITES, config.backends_probability)
assert_not_nil(config.get_backend_config(:stackprof))
assert_nil(config.get_backend_config(:vernier))
end

test "allows passing paths" do
config = Config.new(paths: ["/"])
assert_equal(["/"], config.targets)
end
end
end
end
2 changes: 1 addition & 1 deletion test/app_profiler/sampler/sampler_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class SamplerTest < TestCase
Kernel.stubs(:rand).returns(0.1)
config = Config.new(
sample_rate: 1.0,
paths: ["/foo"],
targets: ["/foo"],
)

request = RequestParameters.new(Rack::Request.new({ "PATH_INFO" => "/foo" }))
Expand Down
Loading