-
Notifications
You must be signed in to change notification settings - Fork 372
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
[PROF-10241] Use Process._fork hook in at_fork
monkey patch on Ruby 3.1+
#3830
Conversation
… 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.
BenchmarksBenchmark execution time: 2024-08-07 16:45:12 Comparing candidate commit 9cf5444 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 23 metrics, 2 unstable metrics. |
|
||
pid = super | ||
|
||
datadog_at_fork_blocks[:child].each(&:call) if datadog_at_fork_blocks.key?(:child) && pid == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would check pid
before key?
for efficiency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9cf5444
# keeps executing code in the child process, killing off the parent, thus effectively replacing it. | ||
# | ||
# This monkey patch makes the `Kernel#datadog_at_fork` mechanism defined above also work in this situation. | ||
# Adds `datadog_at_fork` behavior; see parent module for details. | ||
module ProcessDaemonMonkeyPatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the module now be called just ProcessMoneyPatch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process 💰 Patch 🤣
Fixed in 0c719dd
We're now patching more than just `Process.daemon`.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3830 +/- ##
==========================================
- Coverage 97.87% 97.84% -0.03%
==========================================
Files 1264 1264
Lines 75710 75731 +21
Branches 3716 3719 +3
==========================================
Hits 74098 74098
- Misses 1612 1633 +21 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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 patchKernel
andObject
.To use the new hook we need to monkey patch
Process._fork
instead (see ruby/ruby#5017 andhttps://bugs.ruby-lang.org/issues/17795).
Thus, I've added this monkey patch and disabled patching of
Kernel
andObject
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.
TIP: I recommend reviewing this PR with no whitespace in the diff.
How to test the change?
This change includes test coverage.
Here's a tiny test app that also exercises this feature:
P.s.: You may notice when you run this example on < 3.1 that
BasicFoo.new.call
andBasicObject
are not instrumented!This was actually a known-gap from the old instrumentation.