Disable crashtracking by default

[lloeki]

What does this PR do?

Disable by default as a cautionary measure.

Motivation:

Crashtracking has been identified as causing issues with ECHILD and waitpid, and possibly anything involving SIGCHLD with obscure failure modes of hard to anticipate consequences.

Additional Notes:

• Mitigates Crash tracker spawning long-running child processes? #3954.
• Attempted fix for Process.wait(pid) at [WIP] Fix Process.waitall hanging when crashtracking enabled #3959, but the problem is more intricate.
• Deeper fix libdatadog pending.

How to test the change?

CI

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-10-03 10:04:03

Comparing candidate commit db0565c in PR branch lloeki/disable-crashtracking-by-default with baseline commit 9a012ec in branch master.

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

[ivoanjo]

👍 Change seems reasonable, although the crashtracker specs are failing in CI, I guess they need a bit of fixing given the new default.

[lloeki]

I guess they need a bit of fixing given the new default.

Yes that's what I gathered.

The errors seem weird though, and the pgrep check is a bit naive (on top of requiring pgrep): it would pass if a matching process already existed. Worse, with -f it would pass if any process had the matching string anywhere as argument:

# shell 1
$ ruby -e 'puts "libdatadog-crashtracking-receiver"; sleep 999999'

# shell 2
pgrep -f libdatadog-crashtracking-receiver
3739

So, failures, here they are:

Failed examples:

rspec ./spec/datadog/core/crashtracking/component_spec.rb:368 # Datadog::Core::Crashtracking::Component instance methods integration testing when forked ensures the latest configuration applied
rspec ./spec/datadog/core/crashtracking/component_spec.rb:157 # Datadog::Core::Crashtracking::Component instance methods #start when forked starts a second crash tracker for the fork

  1) Datadog::Core::Crashtracking::Component instance methods integration testing when forked ensures the latest configuration applied
     Failure/Error: expect(status && status.success?).to be(true), "STDOUT:`#{stdout}` STDERR:`#{stderr}"
     
       STDOUT:`` STDERR:`/usr/local/bundle/gems/rspec-support-3.13.1/lib/rspec/support.rb:110:in `block in <module:Support>': (Datadog::Core::Crashtracking::Component (class))._native_start_or_update_on_fork(hash_including(:action=>:update_on_fork, :agent_base_url=>"http://google.com:12345/")) (RSpec::Expectations::ExpectationNotMetError)
           expected: 1 time with arguments: (hash_including(:action=>:update_on_fork, :agent_base_url=>"http://google.com:12345/"))
           received: 0 times

        from /app/spec/datadog/core/crashtracking/component_spec.rb:384:in `block (6 levels) in <top (required)>'

received: 0 times, so OK I get that it's expected because it's disabled by default now, buuuut see below.

  2) Datadog::Core::Crashtracking::Component instance methods #start when forked starts a second crash tracker for the fork
     Got 2 failures:

OK, first one:

     2.1) Failure/Error: expect(status && status.success?).to be(true), "STDOUT:`#{stdout}` STDERR:`#{stderr}"
          
            STDOUT:`` STDERR:`/usr/local/bundle/gems/rspec-support-3.13.1/lib/rspec/support.rb:110:in `block in <module:Support>':  (RSpec::Expectations::ExpectationNotMetError)
            expected #<Integer:5> => 2
                 got #<Integer:3> => 1
          
            Compared using equal?, which compares object identity,
            but expected and actual are not the same object. Use
            `expect(actual).to eq(expected)` if you don't care about
            object identity in this example.

                from /app/spec/datadog/core/crashtracking/component_spec.rb:163:in `block (6 levels) in <top (required)>'

This one's odd because, uh, expect(status && status.success?).to be(true) fails because expected #<Integer:5> => 2 but got #<Integer:3> => 1? That doesn't click. I suppose it's another expectation that raises ExpectationNotMetError but it gets caught by this one.

Second one:

     2.2) Failure/Error: wait_for { `pgrep -f libdatadog-crashtracking-receiver` }.to be_empty
            expected `"1607\n".empty?` to be truthy, got false

          # ./spec/datadog/core/crashtracking/component_spec.rb:104:in `block (3 levels) in <top (required)>'
          # ------------------
          # --- Caused by: ---
          # Timeout::Error:
          #   execution expired
          #   /usr/local/bundle/gems/rspec-wait-0.0.10/lib/rspec/wait/handler.rb:14:in `sleep'

So pgrep has found one match even though it's disabled and should not start? This seems to contradict the very first (expected) failure. It could be matching itself? BSD and GNU behaving differently? Whatever, it's way too brittle.

If anything, the tests passed locally... (huh, they're actually PENDING...)

[lloeki]

Rebased. Sadly some tests needed to be outright removed as they're not reliable.

They'll be fixed in a subsequent PR.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@e5a774a). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #3970   +/-   ##
=========================================
  Coverage          ?   97.84%           
=========================================
  Files             ?     1314           
  Lines             ?    78444           
  Branches          ?     3889           
=========================================
  Hits              ?    76750           
  Misses            ?     1694           
  Partials          ?        0           

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