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] Use Process._fork hook in at_fork monkey patch on Ruby 3.1+ #3830

Merged
merged 3 commits into from
Aug 8, 2024

Commits on Aug 7, 2024

  1. [PROF-10241] Use Process._fork hook in at_fork monkey patch on Ruby…

    … 3.1+
    
    **What does this PR do?**
    
    This PR solves the long-standing "TODO" we had on the `at_fork` monkey
    patch where on Ruby 3.1+ there's a VM hook for instrumenting fork
    operations that avoids us needing to monkey patch `Kernel` and `Object`.
    
    To use the new hook we need to monkey patch `Process._fork` instead
    (see ruby/ruby#5017 and
    https://bugs.ruby-lang.org/issues/17795).
    
    Thus, I've added this monkey patch and disabled patching of
    `Kernel` and `Object` on Ruby 3.1+.
    
    **Motivation:**
    
    Avoid monkey patching core classes that no longer need to.
    
    **Additional Notes:**
    
    I'm not quite happy with either the AtForkMonkeyPatch or its specs, but
    I decided to not do a full revamp for now.
    
    **How to test the change?**
    
    This change includes test coverage.
    
    Here's a tiny test app that also exercises this feature:
    
    ```ruby
    puts RUBY_DESCRIPTION
    
    require 'datadog/core/utils/at_fork_monkey_patch'
    
    Datadog::Core::Utils::AtForkMonkeyPatch.apply!
    
    Process.datadog_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
    ```
    
    P.s.: You may notice when you run this example on < 3.1 that
    `BasicFoo.new.call` and `BasicObject` are not instrumented!
    
    This was actually a known-gap from the old instrumentation.
    ivoanjo committed Aug 7, 2024
    Configuration menu
    Copy the full SHA
    b55c33f View commit details
    Browse the repository at this point in the history
  2. Rename ProcessDaemonMonkeyPatch -> ProcessMonkeyPatch

    We're now patching more than just `Process.daemon`.
    ivoanjo committed Aug 7, 2024
    Configuration menu
    Copy the full SHA
    0c719dd View commit details
    Browse the repository at this point in the history
  3. Minor: Check pid before hash

    ivoanjo committed Aug 7, 2024
    Configuration menu
    Copy the full SHA
    9cf5444 View commit details
    Browse the repository at this point in the history