Skip to content

Commit

Permalink
Refactor: Separate Profiling ThreadContext initialization from CpuAnd…
Browse files Browse the repository at this point in the history
…WallTimeWorker

**What does this PR do?**

This PR separates the initialization of the
`Profiling::Collectors::ThreadContext` component from the initialization
for the `Profiling::Collectors::CpuAndWallTimeWorker`.

Now, both are created in `Datadog::Profiling::Component`.

**Motivation:**

Historically, these two classes have been moving apart, as the
profiler development evolves.

As more settings need to get added to the `ThreadContext`, it became
very awkward to keep changing the `CpuAndWallTimeWorker` to forward
more and more things.

**Additional Notes:**

N/A

**How to test the change?**

The existing test coverage already covers this.
  • Loading branch information
ivoanjo committed Aug 31, 2023
1 parent 85ff055 commit 63e681d
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 56 deletions.
13 changes: 1 addition & 12 deletions lib/datadog/profiling/collectors/cpu_and_wall_time_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,10 @@ class CpuAndWallTimeWorker
public

def initialize(
recorder:,
max_frames:,
tracer:,
endpoint_collection_enabled:,
gc_profiling_enabled:,
allocation_counting_enabled:,
no_signals_workaround_enabled:,
timeline_enabled:,
thread_context_collector: ThreadContext.new(
recorder: recorder,
max_frames: max_frames,
tracer: tracer,
endpoint_collection_enabled: endpoint_collection_enabled,
timeline_enabled: timeline_enabled,
),
thread_context_collector:,
idle_sampling_helper: IdleSamplingHelper.new,
# **NOTE**: This should only be used for testing; disabling the dynamic sampling rate will increase the
# profiler overhead!
Expand Down
7 changes: 5 additions & 2 deletions lib/datadog/profiling/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,15 +79,18 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
cpu_time_enabled: RUBY_PLATFORM.include?('linux'), # Only supported on Linux currently
alloc_samples_enabled: false, # Always disabled for now -- work in progress
)
collector = Datadog::Profiling::Collectors::CpuAndWallTimeWorker.new(
thread_context_collector = Datadog::Profiling::Collectors::ThreadContext.new(
recorder: recorder,
max_frames: settings.profiling.advanced.max_frames,
tracer: optional_tracer,
endpoint_collection_enabled: settings.profiling.advanced.endpoint.collection.enabled,
timeline_enabled: timeline_enabled,
)
collector = Datadog::Profiling::Collectors::CpuAndWallTimeWorker.new(
gc_profiling_enabled: enable_gc_profiling?(settings),
allocation_counting_enabled: settings.profiling.advanced.allocation_counting_enabled,
no_signals_workaround_enabled: no_signals_workaround_enabled,
timeline_enabled: timeline_enabled,
thread_context_collector: thread_context_collector,
)
else
load_pprof_support
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,10 @@ module Datadog
def self._native_allocation_count: () -> ::Integer?

def initialize: (
recorder: Datadog::Profiling::StackRecorder,
max_frames: ::Integer,
tracer: Datadog::Tracing::Tracer?,
endpoint_collection_enabled: bool,
gc_profiling_enabled: bool,
allocation_counting_enabled: bool,
no_signals_workaround_enabled: bool,
timeline_enabled: bool,
?thread_context_collector: Datadog::Profiling::Collectors::ThreadContext,
thread_context_collector: Datadog::Profiling::Collectors::ThreadContext,
?idle_sampling_helper: Datadog::Profiling::Collectors::IdleSamplingHelper,
?dynamic_sampling_rate_enabled: bool,
) -> void
Expand Down
20 changes: 11 additions & 9 deletions spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,10 @@

subject(:cpu_and_wall_time_worker) do
described_class.new(
recorder: recorder,
max_frames: 400,
tracer: nil,
endpoint_collection_enabled: endpoint_collection_enabled,
gc_profiling_enabled: gc_profiling_enabled,
allocation_counting_enabled: allocation_counting_enabled,
no_signals_workaround_enabled: no_signals_workaround_enabled,
timeline_enabled: timeline_enabled,
thread_context_collector: build_thread_context_collector(recorder),
**options
)
end
Expand Down Expand Up @@ -702,13 +698,19 @@ def samples_from_pprof_without_gc_and_overhead(pprof_data)

def build_another_instance
described_class.new(
recorder: build_stack_recorder,
max_frames: 400,
tracer: nil,
endpoint_collection_enabled: endpoint_collection_enabled,
gc_profiling_enabled: gc_profiling_enabled,
allocation_counting_enabled: allocation_counting_enabled,
no_signals_workaround_enabled: no_signals_workaround_enabled,
thread_context_collector: build_thread_context_collector(build_stack_recorder)
)
end

def build_thread_context_collector(recorder)
Datadog::Profiling::Collectors::ThreadContext.new(
recorder: recorder,
max_frames: 400,
tracer: nil,
endpoint_collection_enabled: endpoint_collection_enabled,
timeline_enabled: timeline_enabled,
)
end
Expand Down
46 changes: 19 additions & 27 deletions spec/datadog/profiling/component_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,21 +139,34 @@
build_profiler_component
end

it 'initializes a CpuAndWallTimeWorker collector' do
expect(described_class).to receive(:no_signals_workaround_enabled?).and_return(:no_signals_result)
it 'initializes a ThreadContext collector' do
allow(Datadog::Profiling::Collectors::CpuAndWallTimeWorker).to receive(:new)

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)
expect(settings.profiling.advanced.endpoint.collection)
.to receive(:enabled).and_return(:endpoint_collection_enabled_config)

expect(Datadog::Profiling::Collectors::CpuAndWallTimeWorker).to receive(:new).with(
expect(Datadog::Profiling::Collectors::ThreadContext).to receive(:new).with(
recorder: instance_of(Datadog::Profiling::StackRecorder),
max_frames: :max_frames_config,
tracer: tracer,
endpoint_collection_enabled: anything,
endpoint_collection_enabled: :endpoint_collection_enabled_config,
timeline_enabled: :experimental_timeline_enabled_config,
)

build_profiler_component
end

it 'initializes a CpuAndWallTimeWorker collector' do
expect(described_class).to receive(:no_signals_workaround_enabled?).and_return(:no_signals_result)

expect(Datadog::Profiling::Collectors::CpuAndWallTimeWorker).to receive(:new).with(
gc_profiling_enabled: anything,
allocation_counting_enabled: anything,
no_signals_workaround_enabled: :no_signals_result,
timeline_enabled: :experimental_timeline_enabled_config,
thread_context_collector: instance_of(Datadog::Profiling::Collectors::ThreadContext),
)

build_profiler_component
Expand Down Expand Up @@ -228,28 +241,6 @@
end
end

it 'initializes a CpuAndWallTimeWorker collector with endpoint_collection_enabled set to true' do
expect(Datadog::Profiling::Collectors::CpuAndWallTimeWorker).to receive(:new).with hash_including(
endpoint_collection_enabled: true,
)

build_profiler_component
end

context 'when endpoint_collection_enabled is disabled' do
before do
settings.profiling.advanced.endpoint.collection.enabled = false
end

it 'initializes a CpuAndWallTimeWorker collector with endpoint_collection_enabled set to false' do
expect(Datadog::Profiling::Collectors::CpuAndWallTimeWorker).to receive(:new).with hash_including(
endpoint_collection_enabled: false,
)

build_profiler_component
end
end

it 'sets up the Profiler with the CpuAndWallTimeWorker collector' do
expect(Datadog::Profiling::Profiler).to receive(:new).with(
[instance_of(Datadog::Profiling::Collectors::CpuAndWallTimeWorker)],
Expand All @@ -267,6 +258,7 @@
end

it 'sets up the Exporter internal_metadata with no_signals_workaround_enabled and timeline_enabled settings' do
allow(Datadog::Profiling::Collectors::ThreadContext).to receive(:new)
allow(Datadog::Profiling::Collectors::CpuAndWallTimeWorker).to receive(:new)

expect(described_class).to receive(:no_signals_workaround_enabled?).and_return(:no_signals_result)
Expand Down

0 comments on commit 63e681d

Please sign in to comment.