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.

Sign up for GitHub

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-8942] Enable Ruby timeline by default #3428

Merged
merged 3 commits into from
Feb 21, 2024
Merged

[PROF-8942] Enable Ruby timeline by default #3428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4d3089
[PROF-8942] Enable Ruby timeline by default
ivoanjo Feb 1, 2024
e3210c5
Update with changes from PR #3357
ivoanjo Feb 21, 2024
c862874
Remove unneeded TODO
ivoanjo Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions lib/datadog/core/configuration/settings.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,9 +424,27 @@ def initialize(*_)
#
# @default `DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED` environment variable as a boolean, otherwise `false`
option :experimental_timeline_enabled do |o|
o.after_set do |_, _, precedence|
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly off-topic: I've noticed we don't do this in the other deprecated settings but wonder if we should:

Keep the o.env setting so that we warn if deprecated env variables are left lying around?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an interesting idea...

Although at this point, I was thinking that for 2.0 we'd remove all these settings 🤔. Do you think we should keep them around, potentially with the o.env as well?

Copy link
Contributor

@AlexJF AlexJF Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nah good time to clean up house but I think it'd make sense to take this into account during the lifetime of 2.x and the deprecations that happen therein

unless precedence == Datadog::Core::Configuration::Option::Precedence::DEFAULT
Datadog.logger.warn(
'The profiling.advanced.experimental_timeline_enabled setting has been deprecated for removal ' \
'and no longer does anything. Please remove it from your Datadog.configure block. ' \
'The timeline feature counting is now controlled by the `timeline_enabled` setting instead.'
)
end
end
end

# Controls data collection for the timeline feature.
#
# If you needed to disable this, please tell us why on <https://github.com/DataDog/dd-trace-rb/issues/new>,
# so we can fix it!
#
# @default `DD_PROFILING_TIMELINE_ENABLED` environment variable as a boolean, otherwise `true`
option :timeline_enabled do |o|
o.type :bool
o.env 'DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED'
o.default false
o.env 'DD_PROFILING_TIMELINE_ENABLED'
o.default true
end

# The profiler gathers data by sending `SIGPROF` unix signals to Ruby application threads.
Expand Down
2 changes: 1 addition & 1 deletion lib/datadog/profiling/component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
# NOTE: Please update the Initialization section of ProfilingDevelopment.md with any changes to this method

no_signals_workaround_enabled = no_signals_workaround_enabled?(settings)
timeline_enabled = settings.profiling.advanced.experimental_timeline_enabled
timeline_enabled = settings.profiling.advanced.timeline_enabled
allocation_profiling_enabled = enable_allocation_profiling?(settings)
heap_sample_every = get_heap_sample_every(settings)
heap_profiling_enabled = enable_heap_profiling?(settings, allocation_profiling_enabled, heap_sample_every)
Expand Down
30 changes: 19 additions & 11 deletions spec/datadog/core/configuration/settings_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,20 +723,28 @@
end
end

describe '#experimental_timeline_enabled' do
subject(:experimental_timeline_enabled) { settings.profiling.advanced.experimental_timeline_enabled }
describe '#experimental_timeline_enabled=' do
it 'logs a warning that this no longer does anything' do
expect(Datadog.logger).to receive(:warn).with(/no longer does anything/)

context 'when DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED' do
settings.profiling.advanced.experimental_timeline_enabled = 0
end
end

describe '#timeline_enabled' do
subject(:timeline_enabled) { settings.profiling.advanced.timeline_enabled }

context 'when DD_PROFILING_TIMELINE_ENABLED' do
around do |example|
ClimateControl.modify('DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED' => environment) do
ClimateControl.modify('DD_PROFILING_TIMELINE_ENABLED' => environment) do
example.run
end
end

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 @@ -749,12 +757,12 @@
end
end

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

Expand Down
10 changes: 5 additions & 5 deletions spec/datadog/profiling/component_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@

expect(settings.profiling.advanced).to receive(:max_frames).and_return(:max_frames_config)
expect(settings.profiling.advanced)
.to receive(:experimental_timeline_enabled).and_return(:experimental_timeline_enabled_config)
.to receive(:timeline_enabled).and_return(:timeline_enabled_config)
expect(settings.profiling.advanced.endpoint.collection)
.to receive(:enabled).and_return(:endpoint_collection_enabled_config)

Expand All @@ -67,7 +67,7 @@
max_frames: :max_frames_config,
tracer: tracer,
endpoint_collection_enabled: :endpoint_collection_enabled_config,
timeline_enabled: :experimental_timeline_enabled_config,
timeline_enabled: :timeline_enabled_config,
)

build_profiler_component
Expand Down Expand Up @@ -330,7 +330,7 @@
end

context 'when timeline is enabled' do
before { settings.profiling.advanced.experimental_timeline_enabled = true }
before { settings.profiling.advanced.timeline_enabled = true }

it 'sets up the StackRecorder with timeline_enabled: true' do
expect(Datadog::Profiling::StackRecorder)
Expand All @@ -341,7 +341,7 @@
end

context 'when timeline is disabled' do
before { settings.profiling.advanced.experimental_timeline_enabled = false }
before { settings.profiling.advanced.timeline_enabled = false }

it 'sets up the StackRecorder with timeline_enabled: false' do
expect(Datadog::Profiling::StackRecorder)
Expand Down Expand Up @@ -373,7 +373,7 @@
allow(Datadog::Profiling::StackRecorder).to receive(:new)

expect(described_class).to receive(:no_signals_workaround_enabled?).and_return(:no_signals_result)
expect(settings.profiling.advanced).to receive(:experimental_timeline_enabled).and_return(:timeline_result)
expect(settings.profiling.advanced).to receive(:timeline_enabled).and_return(:timeline_result)
expect(settings.profiling.advanced).to receive(:experimental_heap_sample_rate).and_return(456)
expect(Datadog::Profiling::Exporter).to receive(:new).with(
hash_including(
Expand Down
Loading