Skip to content

Commit

Permalink
[PROF-10275] Enable crashtracking telemetry by default when profiler …
Browse files Browse the repository at this point in the history
…is enabled

**What does this PR do?**

This PR enables the crashtracking telemetry feature by default when the
profiler is enabled.

This feature can be disabled using the
`DD_PROFILING_EXPERIMENTAL_CRASH_TRACKING_ENABLED` environment variable
or the `c.profiling.advanced.experimental_crash_tracking_enabled`
setting via code.

This setting is not very well-named, as the feature is no longer
experimental. But as we're actually considering separating crashtracking
from profiling, I decided to leave the current setting in place until we
can decide where this is going to live, since that's going to influence
where the new setting without "experimental" on its name should land.

**Motivation:**

Crashtracking allows us to identify issues that may otherwise go
unnoticed. We've been deploying it on other Datadog libraries with great
success so far.

**Additional Notes:**

We've had configurations running with crashtracking enabled on our usual
validation environments + almost all (1 remaining!) internal Ruby apps
also has been running with this feature.

We've not seen any noticeable impact for enabling this feature.

In fact, now that it's going to be the default, I'll go ahead and remove
the separate configurations we had for testing it.

**How to test the change?**

The following snippet starts a Ruby instance with profiling, and crashes
it immediately:

```bash
DD_PROFILING_ENABLED=true bundle exec ddprofrb exec ruby -e 'Process.kill("SEGV", Process.pid)'
```

You should see an error report submitted to the agent.
  • Loading branch information
ivoanjo committed Jul 31, 2024
1 parent 47f12db commit c686714
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 19 deletions.
7 changes: 0 additions & 7 deletions .gitlab/benchmarks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,6 @@ only-profiling-heap:
DD_PROFILING_ALLOCATION_ENABLED: "true"
DD_PROFILING_EXPERIMENTAL_HEAP_ENABLED: "true"

only-profiling-crashtracking:
extends: .benchmarks
variables:
DD_BENCHMARKS_CONFIGURATION: only-profiling
DD_PROFILING_ENABLED: "true"
DD_PROFILING_EXPERIMENTAL_CRASH_TRACKING_ENABLED: "true"

profiling-and-tracing:
extends: .benchmarks
variables:
Expand Down
7 changes: 5 additions & 2 deletions lib/datadog/core/configuration/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -453,12 +453,15 @@ def initialize(*_)

# Enables reporting of information when the Ruby VM crashes.
#
# This feature is no longer experimental, and we plan to deprecate this setting and replace it with a
# properly-named one soon.
#
# @default `DD_PROFILING_EXPERIMENTAL_CRASH_TRACKING_ENABLED` environment variable as a boolean,
# otherwise `false`
# otherwise `true`
option :experimental_crash_tracking_enabled do |o|
o.type :bool
o.env 'DD_PROFILING_EXPERIMENTAL_CRASH_TRACKING_ENABLED'
o.default false
o.default true
end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/datadog/profiling/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
# and thus can't really provide a valid configuration to talk to a Datadog agent. Thus, in this situation,
# we can't use the crashtracker, even if enabled.
unless transport.respond_to?(:exporter_configuration)
Datadog.logger.warn(
Datadog.logger.debug(
'Cannot enable profiling crash tracking as a custom settings.profiling.exporter.transport is configured'
)
return
Expand Down
8 changes: 4 additions & 4 deletions spec/datadog/core/configuration/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -828,7 +828,7 @@
context 'is not defined' do
let(:environment) { nil }

it { is_expected.to be false }
it { is_expected.to be true }
end

[true, false].each do |value|
Expand All @@ -843,10 +843,10 @@

describe '#experimental_crash_tracking_enabled=' do
it 'updates the #experimental_crash_tracking_enabled setting' do
expect { settings.profiling.advanced.experimental_crash_tracking_enabled = true }
expect { settings.profiling.advanced.experimental_crash_tracking_enabled = false }
.to change { settings.profiling.advanced.experimental_crash_tracking_enabled }
.from(false)
.to(true)
.from(true)
.to(false)
end
end

Expand Down
8 changes: 3 additions & 5 deletions spec/datadog/profiling/component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -547,8 +547,6 @@
end

context 'when crash tracking is enabled' do
before { settings.profiling.advanced.experimental_crash_tracking_enabled = true }

it 'initializes the crash tracker' do
expect(Datadog::Profiling::Crashtracker).to receive(:new).with(
exporter_configuration: array_including(:agent),
Expand All @@ -564,11 +562,11 @@

before do
settings.profiling.exporter.transport = custom_transport
allow(Datadog.logger).to receive(:warn)
allow(Datadog.logger).to receive(:debug)
end

it 'warns that crash tracking will not be enabled' do
expect(Datadog.logger).to receive(:warn).with(/Cannot enable profiling crash tracking/)
it 'debug logs that crash tracking will not be enabled' do
expect(Datadog.logger).to receive(:debug).with(/Cannot enable profiling crash tracking/)

build_profiler_component
end
Expand Down

0 comments on commit c686714

Please sign in to comment.