Skip to content

Commit

Permalink
Dependency inject tracer instance into profiler
Browse files Browse the repository at this point in the history
As discussed during PR review of #1568, it's problematic to just cache
whatever's in `Datadog.tracer` because we can observe the old value
during component initialization.

To break the loop, let's instead directly initialize the
`TraceIdentifiers::Ddtrace` class with the correct tracer during
component initialization.

IMHO this has a further advantage: it makes it really explict where
there is a tracer-to-profiler dependency whereas previously there was
just a call to `Datadog.tracer` deep in the bowels of the profiler that
could be called at any point.
  • Loading branch information
ivoanjo committed Jun 28, 2021
1 parent 7f25f15 commit f6711e2
Show file tree
Hide file tree
Showing 11 changed files with 39 additions and 61 deletions.
18 changes: 5 additions & 13 deletions docs/ProfilingDevelopment.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,14 @@ flow:
4. The `Setup` task activates our extensions
* `Datadog::Profiling::Ext::Forking`
* `Datadog::Profiling::Ext::CPU`
5. Still inside `Datadog::Components`, the `build_profiler` method then creates and wires up the Profiler:
```ruby
recorder = build_profiler_recorder(settings)
collectors = build_profiler_collectors(settings, recorder)
exporters = build_profiler_exporters(settings)
scheduler = build_profiler_scheduler(settings, recorder, exporters)

Datadog::Profiler.new(collectors, scheduler)
```
5. Still inside `Datadog::Components`, the `build_profiler` method then creates and wires up the Profiler as such:
```asciiflow
+------------+
| Profiler |
+-+--------+-+
| |
v v
+---------+--+ +--+--------+
+-+-------+--+
| |
v v
+---------+--+ +-+---------+
| Collectors | | Scheduler |
+---------+--+ +-+-------+-+
| | |
Expand Down
11 changes: 7 additions & 4 deletions lib/ddtrace/configuration/components.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def build_tracer(settings, agent_settings)
tracer
end

def build_profiler(settings, agent_settings)
def build_profiler(settings, agent_settings, tracer)
return unless Datadog::Profiling.supported? && settings.profiling.enabled

unless defined?(Datadog::Profiling::Tasks::Setup)
Expand Down Expand Up @@ -107,8 +107,10 @@ def build_profiler(settings, agent_settings)

# NOTE: Please update the Initialization section of ProfilingDevelopment.md with any changes to this method

trace_identifiers_helper = Datadog::Profiling::TraceIdentifiers::Helper.new(tracer: tracer)

recorder = build_profiler_recorder(settings)
collectors = build_profiler_collectors(settings, recorder)
collectors = build_profiler_collectors(settings, recorder, trace_identifiers_helper)
exporters = build_profiler_exporters(settings, agent_settings)
scheduler = build_profiler_scheduler(settings, recorder, exporters)

Expand Down Expand Up @@ -164,10 +166,11 @@ def build_profiler_recorder(settings)
Datadog::Profiling::Recorder.new(event_classes, settings.profiling.max_events)
end

def build_profiler_collectors(settings, recorder)
def build_profiler_collectors(settings, recorder, trace_identifiers_helper)
[
Datadog::Profiling::Collectors::Stack.new(
recorder,
trace_identifiers_helper: trace_identifiers_helper,
max_frames: settings.profiling.max_frames
# TODO: Provide proc that identifies Datadog worker threads?
# ignore_thread: settings.profiling.ignore_profiler
Expand Down Expand Up @@ -209,7 +212,7 @@ def initialize(settings)
@tracer = self.class.build_tracer(settings, agent_settings)

# Profiler
@profiler = self.class.build_profiler(settings, agent_settings)
@profiler = self.class.build_profiler(settings, agent_settings, @tracer)

# Runtime metrics
@runtime_metrics = self.class.build_runtime_metrics_worker(settings)
Expand Down
1 change: 1 addition & 0 deletions lib/ddtrace/profiling.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ def self.load_profiling
require 'ddtrace/profiling/transport/io'
require 'ddtrace/profiling/transport/http'
require 'ddtrace/profiling/profiler'
require 'ddtrace/profiling/trace_identifiers/helper'

require 'ddtrace/profiling/pprof/pprof_pb'

Expand Down
10 changes: 5 additions & 5 deletions lib/ddtrace/profiling/collectors/stack.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
require 'ddtrace/profiling/backtrace_location'
require 'ddtrace/profiling/trace_identifiers/helper'
require 'ddtrace/profiling/events/stack'
require 'ddtrace/utils/only_once'
require 'ddtrace/utils/time'
Expand All @@ -12,7 +11,7 @@ module Collectors
# Collects stack trace samples from Ruby threads for both CPU-time (if available) and wall-clock.
# Runs on its own background thread.
#
class Stack < Worker
class Stack < Worker # rubocop:disable Metrics/ClassLength
include Workers::Polling

DEFAULT_MAX_TIME_USAGE_PCT = 2.0
Expand All @@ -22,27 +21,28 @@ class Stack < Worker
attr_reader \
:recorder,
:max_frames,
:trace_identifiers_helper,
:ignore_thread,
:max_time_usage_pct,
:thread_api

def initialize(
recorder,
max_frames:,
trace_identifiers_helper:, # Usually an instance of Datadog::Profiling::TraceIdentifiers::Helper
ignore_thread: nil,
max_time_usage_pct: DEFAULT_MAX_TIME_USAGE_PCT,
thread_api: Thread,
trace_identifiers_helper: Datadog::Profiling::TraceIdentifiers::Helper.new,
fork_policy: Workers::Async::Thread::FORK_POLICY_RESTART, # Restart in forks by default
interval: MIN_INTERVAL,
enabled: true
)
@recorder = recorder
@max_frames = max_frames
@trace_identifiers_helper = trace_identifiers_helper
@ignore_thread = ignore_thread
@max_time_usage_pct = max_time_usage_pct
@thread_api = thread_api
@trace_identifiers_helper = trace_identifiers_helper

# Workers::Async::Thread settings
self.fork_policy = fork_policy
Expand Down Expand Up @@ -125,7 +125,7 @@ def collect_thread_event(thread, wall_time_interval_ns)
locations = convert_backtrace_locations(locations)

thread_id = thread.respond_to?(:pthread_thread_id) ? thread.pthread_thread_id : thread.object_id
trace_id, span_id = @trace_identifiers_helper.trace_identifiers_for(thread)
trace_id, span_id = trace_identifiers_helper.trace_identifiers_for(thread)
cpu_time = get_cpu_time_interval!(thread)

Events::StackSample.new(
Expand Down
22 changes: 3 additions & 19 deletions lib/ddtrace/profiling/trace_identifiers/ddtrace.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,34 +7,18 @@ module TraceIdentifiers
# given thread, if there is an active trace for that thread in Datadog.tracer.
class Ddtrace
def initialize(tracer: nil)
@tracer = tracer
@tracer = (tracer if tracer.respond_to?(:active_correlation))
end

def trace_identifiers_for(thread)
current_tracer = tracer
return unless current_tracer
return unless @tracer

correlation = current_tracer.active_correlation(thread)
correlation = @tracer.active_correlation(thread)
trace_id = correlation.trace_id
span_id = correlation.span_id

[trace_id, span_id] if trace_id && trace_id != 0 && span_id && span_id != 0
end

private

def tracer
return @tracer if @tracer

# NOTE: Because the profiler may start working concurrently with tracer initialization,
# we need to be defensive here.
return unless Datadog.respond_to?(:tracer)

tracer = Datadog.tracer
return unless tracer.respond_to?(:active_correlation)

@tracer = tracer
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/ddtrace/profiling/trace_identifiers/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class Helper
].freeze
private_constant :DEFAULT_SUPPORTED_APIS

def initialize(supported_apis: DEFAULT_SUPPORTED_APIS.map(&:new))
def initialize(tracer:, supported_apis: DEFAULT_SUPPORTED_APIS.map { |api| api.new(tracer: tracer) })
@supported_apis = supported_apis
end

Expand Down
2 changes: 1 addition & 1 deletion lib/ddtrace/profiling/trace_identifiers/opentelemetry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class OpenTelemetry
UNSUPPORTED_VERSION_ONLY_ONCE = Datadog::Utils::OnlyOnce.new
private_constant :UNSUPPORTED_VERSION_ONLY_ONCE

def initialize
def initialize(**_)
@available = false
@checked_version = false
@current_context_key = nil
Expand Down
5 changes: 3 additions & 2 deletions spec/ddtrace/configuration/components_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
.and_return(tracer)

expect(described_class).to receive(:build_profiler)
.with(settings, instance_of(Datadog::Configuration::AgentSettingsResolver::AgentSettings))
.with(settings, instance_of(Datadog::Configuration::AgentSettingsResolver::AgentSettings), tracer)
.and_return(profiler)

expect(described_class).to receive(:build_runtime_metrics_worker)
Expand Down Expand Up @@ -666,8 +666,9 @@
describe '::build_profiler' do
let(:agent_settings) { Datadog::Configuration::AgentSettingsResolver.call(settings, logger: nil) }
let(:profiler) { build_profiler }
let(:tracer) { instance_double(Datadog::Tracer) }

subject(:build_profiler) { described_class.build_profiler(settings, agent_settings) }
subject(:build_profiler) { described_class.build_profiler(settings, agent_settings, tracer) }

context 'when profiling is not supported' do
before { allow(Datadog::Profiling).to receive(:supported?).and_return(false) }
Expand Down
7 changes: 5 additions & 2 deletions spec/ddtrace/profiling/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
skip 'Profiling is not supported.' unless Datadog::Profiling.supported?
end

let(:tracer) { instance_double(Datadog::Tracer) }

shared_context 'StackSample events' do
# NOTE: Please do not convert stack_one or stack_two to let, because
# we want the method names on the resulting stacks to be stack_one or
Expand Down Expand Up @@ -55,7 +57,7 @@ def stack_two
let(:collector) do
Datadog::Profiling::Collectors::Stack.new(
recorder,
enabled: true,
trace_identifiers_helper: Datadog::Profiling::TraceIdentifiers::Helper.new(tracer: tracer),
max_frames: 400
)
end
Expand Down Expand Up @@ -121,10 +123,11 @@ def stack_two
@current_span = span
example.run
end

Datadog.tracer.shutdown!
end

let(:tracer) { Datadog.tracer }

before do
expect(recorder)
.to receive(:flush)
Expand Down
20 changes: 7 additions & 13 deletions spec/ddtrace/profiling/trace_identifiers/ddtrace_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,16 @@
context 'when no tracer instance is available' do
let(:tracer) { nil }

context 'because the tracer is unavailable' do
let(:datadog) { Module.new { const_set('Utils', Datadog::Utils) } }

before { stub_const('Datadog', datadog, transfer_nested_constant: true) }

it do
expect(trace_identifiers_for).to be nil
end
it do
expect(trace_identifiers_for).to be nil
end
end

context 'because correlations are unavailable' do
before { allow(Datadog).to receive(:tracer).and_return(instance_double(Datadog::Tracer)) }
context 'when tracer does not support #active_correlation' do
let(:tracer) { double('Tracer') } # rubocop:disable RSpec/VerifiedDoubles

it do
expect(trace_identifiers_for).to be nil
end
it do
expect(trace_identifiers_for).to be nil
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/ddtrace/profiling/trace_identifiers/helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
let(:api1) { instance_double(Datadog::Profiling::TraceIdentifiers::Ddtrace, 'api1') }
let(:api2) { instance_double(Datadog::Profiling::TraceIdentifiers::Ddtrace, 'api2') }

subject(:trace_identifiers_helper) { described_class.new(supported_apis: [api1, api2]) }
subject(:trace_identifiers_helper) { described_class.new(tracer: nil, supported_apis: [api1, api2]) }

describe '::DEFAULT_SUPPORTED_APIS' do
it 'contains the Datadog and OpenTelemetry trace identifiers' do
Expand Down

0 comments on commit f6711e2

Please sign in to comment.