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

[PROF-10241] Fix issue in tests by simplifying at_fork monkey patch #3834

Merged
merged 2 commits into from
Aug 12, 2024

Commits on Aug 8, 2024

  1. [PROF-10241] Fix issue in tests by simplifying at_fork monkey patch

    **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
    ```
    ivoanjo committed Aug 8, 2024
    Configuration menu
    Copy the full SHA
    3dc71fb View commit details
    Browse the repository at this point in the history

Commits on Aug 12, 2024

  1. Configuration menu
    Copy the full SHA
    7642788 View commit details
    Browse the repository at this point in the history