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] Extract profiler at_fork monkey patch to utils #3829

Merged
merged 13 commits into from
Aug 7, 2024

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Aug 7, 2024

What does this PR do?

This PR extracts the profiler Datadog::Profiling::Ext::Forking monkey patch to now live under Datadog::Core::Utils::AtForkMonkeyPatch.

It also does a clean up pass on some of the comments and specs, as well as rename some of the modules used for the actual monkey patching.

Motivation:

This monkey patch provides a datadog_at_fork mechanism that can be used to run code just after a process gets forked.

Here's a quick example:

$ bundle exec pry
[1] pry(main)> require 'datadog/core/utils/at_fork_monkey_patch'
=> true
[2] pry(main)> Datadog::Core::Utils::AtForkMonkeyPatch.apply!
=> true
[3] pry(main)> Process.datadog_at_fork(:child) { puts "This code is running in a child process!" }
=> nil
[4] pry(main)> fork { puts "Hello from fork" }
This code is running in a child process!
Hello from fork
=> 447306

Up until now, only the profiler needed this behavior of getting auto-started right after a process forks.

Every other component of the datadog gem lazily waited until it got called again (e.g. the tracing bits would not get re-initialized until you tried to call the tracer).

With the introduction of the crashtracker, we also want the crashtracker to auto-start after a fork, so I'm extracting the monkey patch so we can use it on that component as well.

Additional Notes:

I'm preparing a follow-up PR to add support for Process._fork.
It's been a long time coming, and it's not that big of a change.

How to test the change?

Our existing specs already cover this change. Also see the example above.

This typechecking skeleton doesn't actually work, so let's remove
it until we can write a proper one.
**What does this PR do?**

This PR extracts the profiler `Datadog::Profiling::Ext::Forking` monkey
patch to now live under `Datadog::Core::Utils::AtForkMonkeyPatch`.

It also does a clean up pass on some of the comments and specs, as well
as rename some of the modules used for the actual monkey patching.

**Motivation:**

This monkey patch provides a `datadog_at_fork` mechanism that
can be used to run code just after a process gets forked.

Here's a quick example:

```ruby
$ bundle exec pry
[1] pry(main)> require 'datadog/core/utils/at_fork_monkey_patch'
=> true
[2] pry(main)> Datadog::Core::Utils::AtForkMonkeyPatch.apply!
=> true
[3] pry(main)> Process.datadog_at_fork(:child) { puts "This code is running in a child process!" }
=> nil
[4] pry(main)> fork { puts "Hello from fork" }
This code is running in a child process!
Hello from fork
=> 447306
```

Up until now, only the profiler needed this behavior of getting
auto-started right after a process forks.

Every other component of ddtrace lazily waited until it got called
again (e.g. the tracing bits would not get re-initialized until
you tried to call the tracer).

With the introduction of the crashtracker, we also want the
crashtracker to auto-start after a fork, so I'm extracing the
monkey patch so we can use it on that component as well.

**Additional Notes:**

I'm preparing a follow-up PR to add support for `Process._fork`.
It's been a long time coming, and it's not that big of a change.

**How to test the change?**

Our existing specs already cover this change. Also see the example
above.
I did not realize we had a check for this ;)
@ivoanjo ivoanjo requested review from a team as code owners August 7, 2024 10:44
@github-actions github-actions bot added core Involves Datadog core libraries profiling Involves Datadog profiling labels Aug 7, 2024
@ivoanjo ivoanjo requested a review from TonyCTHsu August 7, 2024 10:44
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (0ba87b9) to head (4c69eb3).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3829   +/-   ##
=======================================
  Coverage   97.86%   97.87%           
=======================================
  Files        1264     1264           
  Lines       75762    75710   -52     
  Branches     3720     3716    -4     
=======================================
- Hits        74146    74098   -48     
+ Misses       1616     1612    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Aug 7, 2024

Benchmarks

Benchmark execution time: 2024-08-07 12:11:21

Comparing candidate commit 4c69eb3 in PR branch ivoanjo/prof-10241-extract-fork-monkeypatch with baseline commit 0ba87b9 in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics.

scenario:Gem loading

  • 🟥 throughput [-0.096op/s; -0.094op/s] or [-3.206%; -3.141%]

context 'and returns from the parent context' do
# By setting the fork result = integer, we're
# simulating #fork running in the parent process.
let(:fork_result) { rand(100) }

Choose a reason for hiding this comment

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

Code Vulnerability

Do not use random, prefer the SecureRandom module (...read more)

The "Avoid Random" rule is focused on discouraging the use of the rand method with negative numbers or without any arguments. This is because rand without arguments returns a floating-point number between 0 and 1, which can lead to unpredictable results and make the code harder to test and debug. Moreover, using rand with negative numbers is not allowed and will raise an error.

This rule is important because it promotes the use of predictable and testable code. Randomness in code can lead to inconsistent behavior, which makes it more difficult to identify and fix bugs. Additionally, the use of a random number generator without a defined range or with a negative range can lead to unexpected results or runtime errors, respectively.

To avoid this, always use rand with a positive integer argument to define the range of the random numbers that can be generated. This ensures that the output is predictable and within a specific range. For example, use rand(100) to generate a random number between 0 and 99. If you need a random floating-point number within a specific range, you can use rand in combination with Range#to_a, like rand(1.0..10.0). This will generate a random floating-point number between 1.0 and 10.0.

View in Datadog  Leave us feedback  Documentation

@ivoanjo ivoanjo force-pushed the ivoanjo/prof-10241-extract-fork-monkeypatch branch from b55c33f to 4c69eb3 Compare August 7, 2024 11:35
# rubocop:disable Style/ClassVars
@@datadog_at_fork_blocks ||= {}
# rubocop:enable Style/ClassVars
@@datadog_at_fork_blocks ||= {} # rubocop:disable Style/ClassVars
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be protected by a mutex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's an excellent point. When this code runs the first time we shouldn't see concurrent forking, but "shouldn't" is not "won't".

Since we want to ship some of these changes as soon as possible, I've gone ahead and created #3831 and I'll come back to refactor this in a few days :)

@ivoanjo ivoanjo merged commit 1db5ae9 into master Aug 7, 2024
323 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/prof-10241-extract-fork-monkeypatch branch August 7, 2024 16:04
@github-actions github-actions bot added this to the 2.3.0 milestone Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries profiling Involves Datadog profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants