[PROF-10241] Fix issue in tests by simplifying at_fork monkey patch
#3834
Conversation
**What does this PR do?**
This PR fixes an issue that we identified in the
`profiling/tasks/setup_spec.rb` that was triggered by applying the
at_fork monkey patch before running that spec.
This issue could be triggered by adding
```ruby
Datadog::Core::Utils::AtForkMonkeyPatch.apply!
```
to the top of the `setup_spec.rb` (before the `RSpec.describe`).
```
Datadog::Profiling::Tasks::Setup
#run
actives the forking extension before setting up the at_fork hooks
only sets up the extensions and hooks once, even across different instances
#setup_at_fork_hooks
when Process#datadog_at_fork is available
sets up an at_fork hook that restarts the profiler (FAILED - 1)
when there is an issue starting the profiler
logs an exception (FAILED - 2)
does not raise any error (FAILED - 3)
when #datadog_at_fork is not available
logs a debug message
Failures:
1) Datadog::Profiling::Tasks::Setup#setup_at_fork_hooks when Process#datadog_at_fork is available sets up an at_fork hook that restarts the profiler
Failure/Error:
example.run.tap do
tracer_shutdown!
end
NameError:
undefined method `datadog_at_fork' for class `#<Class:Process>'
object_singleton_class.__send__(@original_visibility, method_name)
^^^^^^^^^
# ./spec/spec_helper.rb:230:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:115:in `block (2 levels) in <top (required)>'
# webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
# rspec-wait-0.0.10/lib/rspec/wait.rb:47:in `block (2 levels) in <top (required)>'
2) Datadog::Profiling::Tasks::Setup#setup_at_fork_hooks when Process#datadog_at_fork is available when there is an issue starting the profiler logs an exception
Failure/Error: at_fork_hook.call
NoMethodError:
undefined method `call' for nil:NilClass
at_fork_hook.call
^^^^^
# ./spec/datadog/profiling/tasks/setup_spec.rb:84:in `block (5 levels) in <top (required)>'
# ./spec/spec_helper.rb:230:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:115:in `block (2 levels) in <top (required)>'
# webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
# rspec-wait-0.0.10/lib/rspec/wait.rb:47:in `block (2 levels) in <top (required)>'
3) Datadog::Profiling::Tasks::Setup#setup_at_fork_hooks when Process#datadog_at_fork is available when there is an issue starting the profiler does not raise any error
Failure/Error: at_fork_hook.call
NoMethodError:
undefined method `call' for nil:NilClass
at_fork_hook.call
^^^^^
# ./spec/datadog/profiling/tasks/setup_spec.rb:76:in `block (5 levels) in <top (required)>'
# ./spec/spec_helper.rb:230:in `block (2 levels) in <top (required)>'
# ./spec/spec_helper.rb:115:in `block (2 levels) in <top (required)>'
# webmock-3.23.1/lib/webmock/rspec.rb:39:in `block (2 levels) in <top (required)>'
# rspec-wait-0.0.10/lib/rspec/wait.rb:47:in `block (2 levels) in <top (required)>'
```
This issue was caused by an incompatibility between rspec-mocks (not
entirely sure if it's a bug) and the monkey patching we were doing.
Rather than fighting with more monkey patch weirdness or rspec,
I decided to fix the issue by simplifying the at fork monkey patch
module by making it as minimal as possible:
* It no longer places extra unneeded functions in Ruby classes, e.g.
`datadog_at_fork_blocks` and `datadog_at_fork`
* It no longer stores its data in a weird class var
This enabled a bunch of simplifications, and the affected
spec was fixed as a side-effect of this.
**Motivation:**
Get the at_fork monkey patch in shape to be used by the crashtracker as
well.
**Additional Notes:**
I was tempted to do a larger refactoring of this module when I was
extracting it, but I decided to aim for a smaller diff.
Since I needed to dive in again to fix the issue with the spec, I
decided to finally go for it.
**How to test the change?**
These changes include test coverage.
Here's a tiny app that can be used to experiment with this:
```ruby
puts RUBY_DESCRIPTION
require 'datadog/core/utils/at_fork_monkey_patch'
Datadog::Core::Utils::AtForkMonkeyPatch.apply!
Datadog::Core::Utils::AtForkMonkeyPatch.at_fork(:child) { puts "Hello from child process: #{Process.pid} "}
puts "Parent pid: #{Process.pid}"
puts "fork { }"
fork { }
Process.wait
puts "Kernel.fork { }"
Kernel.fork { }
Process.wait
puts "Process.fork { }"
Process.fork { }
Process.wait
puts "Foo.new.call"
class Foo
def call
fork { }
end
def self.do_fork
fork { }
end
end
Foo.new.call
Process.wait
puts "Foo.do_fork"
Foo.do_fork
Process.wait
puts "Fork from outside"
Foo.new.send(:fork) { }
Process.wait
class BasicFoo < BasicObject
include ::Kernel
def call
fork { }
end
def self.do_fork
fork { }
end
end
puts "BasicFoo.new.call"
BasicFoo.new.call
Process.wait
puts "BasicObject:"
Class.new(BasicObject) { include(::Kernel); def call; fork { }; end }.new.call
Process.wait
puts "BasicFork.do_fork"
BasicFoo.do_fork
Process.wait
```
github-actions
bot
added
core
|
|
||
| expect(process_module.daemon).to be :process_daemon_result | ||
| it 'passes any arguments to Process.daemon and returns its results' do | ||
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) |
There was a problem hiding this comment.
⚪ Code Quality Violation
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) | |
| expect(process_module.daemon(:arg1, :arg2)).to eq(%i[arg1 arg2]) |
Consider using the %i syntax instead (...read more)
The rule "Prefer %i to the literal array syntax" is a guideline that encourages the use of the %i syntax for arrays of symbols. This is a part of the Ruby style guide that aims to promote conciseness and readability.
Symbols are immutable, reusable objects often used in Ruby instead of strings when the value does not need to be changed. When declaring an array of symbols, using the %i syntax can make your code cleaner and easier to read.
To adhere to this rule, instead of declaring an array of symbols using the literal array syntax like [:foo, :bar, :baz], use the %i syntax like %i[foo bar baz]. It's a good practice to consistently use %i for arrays of symbols as it enhances code readability and maintainability.
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
|
|
||
| expect(process_module.daemon).to be :process_daemon_result | ||
| it 'passes any arguments to Process.daemon and returns its results' do | ||
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) |
There was a problem hiding this comment.
⚪ Code Quality Violation
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) | |
| expect(process_module.daemon(:arg1, :arg2)).to eq(%i[arg1 arg2]) |
Consider using the %i syntax instead (...read more)
The rule "Prefer %i to the literal array syntax" is a guideline that encourages the use of the %i syntax for arrays of symbols. This is a part of the Ruby style guide that aims to promote conciseness and readability.
Symbols are immutable, reusable objects often used in Ruby instead of strings when the value does not need to be changed. When declaring an array of symbols, using the %i syntax can make your code cleaner and easier to read.
To adhere to this rule, instead of declaring an array of symbols using the literal array syntax like [:foo, :bar, :baz], use the %i syntax like %i[foo bar baz]. It's a good practice to consistently use %i for arrays of symbols as it enhances code readability and maintainability.
|
|
||
| expect(process_module.daemon).to be :process_daemon_result | ||
| it 'passes any arguments to Process.daemon and returns its results' do | ||
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) |
There was a problem hiding this comment.
⚪ Code Quality Violation
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) | |
| expect(process_module.daemon(:arg1, :arg2)).to eq(%i[arg1 arg2]) |
Consider using the %i syntax instead (...read more)
The rule "Prefer %i to the literal array syntax" is a guideline that encourages the use of the %i syntax for arrays of symbols. This is a part of the Ruby style guide that aims to promote conciseness and readability.
Symbols are immutable, reusable objects often used in Ruby instead of strings when the value does not need to be changed. When declaring an array of symbols, using the %i syntax can make your code cleaner and easier to read.
To adhere to this rule, instead of declaring an array of symbols using the literal array syntax like [:foo, :bar, :baz], use the %i syntax like %i[foo bar baz]. It's a good practice to consistently use %i for arrays of symbols as it enhances code readability and maintainability.
|
|
||
| expect(process_module.daemon).to be :process_daemon_result | ||
| it 'passes any arguments to Process.daemon and returns its results' do | ||
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) |
There was a problem hiding this comment.
⚪ Code Quality Violation
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) | |
| expect(process_module.daemon(:arg1, :arg2)).to eq(%i[arg1 arg2]) |
Consider using the %i syntax instead (...read more)
The rule "Prefer %i to the literal array syntax" is a guideline that encourages the use of the %i syntax for arrays of symbols. This is a part of the Ruby style guide that aims to promote conciseness and readability.
Symbols are immutable, reusable objects often used in Ruby instead of strings when the value does not need to be changed. When declaring an array of symbols, using the %i syntax can make your code cleaner and easier to read.
To adhere to this rule, instead of declaring an array of symbols using the literal array syntax like [:foo, :bar, :baz], use the %i syntax like %i[foo bar baz]. It's a good practice to consistently use %i for arrays of symbols as it enhances code readability and maintainability.
|
|
||
| expect(process_module.daemon).to be :process_daemon_result | ||
| it 'passes any arguments to Process.daemon and returns its results' do | ||
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) |
There was a problem hiding this comment.
⚪ Code Quality Violation
| expect(process_module.daemon(:arg1, :arg2)).to eq([:arg1, :arg2]) | |
| expect(process_module.daemon(:arg1, :arg2)).to eq(%i[arg1 arg2]) |
Consider using the %i syntax instead (...read more)
The rule "Prefer %i to the literal array syntax" is a guideline that encourages the use of the %i syntax for arrays of symbols. This is a part of the Ruby style guide that aims to promote conciseness and readability.
Symbols are immutable, reusable objects often used in Ruby instead of strings when the value does not need to be changed. When declaring an array of symbols, using the %i syntax can make your code cleaner and easier to read.
To adhere to this rule, instead of declaring an array of symbols using the literal array syntax like [:foo, :bar, :baz], use the %i syntax like %i[foo bar baz]. It's a good practice to consistently use %i for arrays of symbols as it enhances code readability and maintainability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3834 +/- ##
==========================================
- Coverage 97.84% 97.83% -0.01%
==========================================
Files 1264 1264
Lines 75712 75678 -34
Branches 3719 3720 +1
==========================================
- Hits 74079 74042 -37
- Misses 1633 1636 +3 ☔ View full report in Codecov by Sentry. |
BenchmarksBenchmark execution time: 2024-08-12 08:54:08 Comparing candidate commit 7642788 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 21 metrics, 2 unstable metrics. scenario:Allocations (profiling disabled)
scenario:Allocations (profiling enabled)
|
What does this PR do?
This PR fixes an issue that we identified in the
profiling/tasks/setup_spec.rbthat was triggered by applying the at_fork monkey patch before running that spec.This issue could be triggered by adding
to the top of the
setup_spec.rb(before theRSpec.describe).This issue was caused by an incompatibility between rspec-mocks (not entirely sure if it's a bug) and the monkey patching we were doing.
Rather than fighting with more monkey patch weirdness or rspec, I decided to fix the issue by simplifying the at fork monkey patch module by making it as minimal as possible:
datadog_at_fork_blocksanddatadog_at_forkThis enabled a bunch of simplifications, and the affected spec was fixed as a side-effect of this.
Motivation:
Get the at_fork monkey patch in shape to be used by the crashtracker as well.
Additional Notes:
I was tempted to do a larger refactoring of this module when I was extracting it, but I decided to aim for a smaller diff.
Since I needed to dive in again to fix the issue with the spec, I decided to finally go for it.
I suggest reviewing the diff without whitespace differences
How to test the change?
These changes include test coverage.
Here's a tiny app that can be used to experiment with this: