Skip to content

Commit

Permalink
Remove configuration for priority_sampling
Browse files Browse the repository at this point in the history
  • Loading branch information
marcotc committed Dec 12, 2023
1 parent c7ef57c commit 2d1e3c0
Show file tree
Hide file tree
Showing 9 changed files with 42 additions and 177 deletions.
1 change: 0 additions & 1 deletion lib/datadog/core/telemetry/collector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ def tracer_time
'tracing.log_injection',
'tracing.partial_flush.enabled',
'tracing.partial_flush.min_spans_threshold',
'tracing.priority_sampling',
'tracing.report_hostname',
'tracing.sampling.default_rate',
'tracing.sampling.rate_limit'
Expand Down
65 changes: 17 additions & 48 deletions lib/datadog/tracing/component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,63 +70,32 @@ def build_trace_flush(settings)
end
end

# TODO: Sampler should be a top-level component.
# It is currently part of the Tracer initialization
# process, but can take a variety of options (including
# a fully custom instance) that makes the Tracer
# initialization process complex.
def build_sampler(settings)
# A custom sampler is provided
if (sampler = settings.tracing.sampler)
if settings.tracing.priority_sampling == false
sampler
else
ensure_priority_sampling(sampler, settings)
end
elsif (rules = settings.tracing.sampling.rules)
return sampler
end

# Sampling rules are provided
if (rules = settings.tracing.sampling.rules)
post_sampler = Tracing::Sampling::RuleSampler.parse(
rules,
settings.tracing.sampling.rate_limit,
settings.tracing.sampling.default_rate
)

post_sampler ||= # Fallback RuleSampler in case `rules` parsing fails
Tracing::Sampling::RuleSampler.new(
rate_limit: settings.tracing.sampling.rate_limit,
default_sample_rate: settings.tracing.sampling.default_rate
)

Tracing::Sampling::PrioritySampler.new(
base_sampler: Tracing::Sampling::AllSampler.new,
post_sampler: post_sampler
)
elsif settings.tracing.priority_sampling == false
Tracing::Sampling::RuleSampler.new(
rate_limit: settings.tracing.sampling.rate_limit,
default_sample_rate: settings.tracing.sampling.default_rate
)
else
Tracing::Sampling::PrioritySampler.new(
base_sampler: Tracing::Sampling::AllSampler.new,
post_sampler: Tracing::Sampling::RuleSampler.new(
rate_limit: settings.tracing.sampling.rate_limit,
default_sample_rate: settings.tracing.sampling.default_rate
)
)
end
end

def ensure_priority_sampling(sampler, settings)
if sampler.is_a?(Tracing::Sampling::PrioritySampler)
sampler
else
Tracing::Sampling::PrioritySampler.new(
base_sampler: sampler,
post_sampler: Tracing::Sampling::RuleSampler.new(
rate_limit: settings.tracing.sampling.rate_limit,
default_sample_rate: settings.tracing.sampling.default_rate
)
)
end
# The default sampler.
# Used if no custom sampler is provided, or if sampling rule parsing fails.
post_sampler ||= Tracing::Sampling::RuleSampler.new(
rate_limit: settings.tracing.sampling.rate_limit,
default_sample_rate: settings.tracing.sampling.default_rate
)

Tracing::Sampling::PrioritySampler.new(
base_sampler: Tracing::Sampling::AllSampler.new,
post_sampler: post_sampler
)
end

# TODO: Writer should be a top-level component.
Expand Down
6 changes: 0 additions & 6 deletions lib/datadog/tracing/configuration/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -227,12 +227,6 @@ def self.extended(base)
option :min_spans_threshold, default: 500, type: :int
end

# Enables {https://docs.datadoghq.com/tracing/trace_retention_and_ingestion/#datadog-intelligent-retention-filter
# Datadog intelligent retention filter}.
# @default `true`
# @return [Boolean,nil]
option :priority_sampling

option :report_hostname do |o|
o.env Tracing::Configuration::Ext::NET::ENV_REPORT_HOSTNAME
o.default false
Expand Down
6 changes: 0 additions & 6 deletions lib/datadog/tracing/diagnostics/environment_logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ def collect_config!
sampling_rules: sampling_rules,
integrations_loaded: integrations_loaded,
partial_flushing_enabled: partial_flushing_enabled,
priority_sampling_enabled: priority_sampling_enabled,
**instrumented_integrations_settings
}
end
Expand Down Expand Up @@ -129,11 +128,6 @@ def partial_flushing_enabled
!!Datadog.configuration.tracing.partial_flush.enabled
end

# @return [Boolean, nil] priority sampling enabled in configuration
def priority_sampling_enabled
!!Datadog.configuration.tracing.priority_sampling
end

private

def instrumented_integrations
Expand Down
2 changes: 1 addition & 1 deletion sig/datadog/core/telemetry/collector.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Datadog

private

TARGET_OPTIONS: ::Array["ci.enabled" | "logger.level" | "profiling.advanced.code_provenance_enabled" | "profiling.advanced.endpoint.collection.enabled" | "profiling.enabled" | "runtime_metrics.enabled" | "tracing.analytics.enabled" | "tracing.distributed_tracing.propagation_inject_style" | "tracing.distributed_tracing.propagation_extract_style" | "tracing.enabled" | "tracing.log_injection" | "tracing.partial_flush.enabled" | "tracing.partial_flush.min_spans_threshold" | "tracing.priority_sampling" | "tracing.report_hostname" | "tracing.sampling.default_rate" | "tracing.sampling.rate_limit"]
TARGET_OPTIONS: ::Array[::String]

def additional_payload_variables: () -> untyped

Expand Down
70 changes: 7 additions & 63 deletions spec/datadog/core/configuration/components_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -572,74 +572,18 @@
end
end

context 'with :priority_sampling' do
context 'with :sampler' do
before do
allow(settings.tracing)
.to receive(:priority_sampling)
.and_return(priority_sampling)
.to receive(:sampler)
.and_return(sampler)
end

context 'enabled' do
let(:priority_sampling) { true }
let(:sampler) { double('sampler') }

it_behaves_like 'new tracer'

context 'with :sampler' do
before do
allow(settings.tracing)
.to receive(:sampler)
.and_return(sampler)
end

context 'that is a priority sampler' do
let(:sampler) { Datadog::Tracing::Sampling::PrioritySampler.new }

it_behaves_like 'new tracer' do
let(:options) { { sampler: sampler } }
it_behaves_like 'event publishing writer and priority sampler'
end
end

context 'that is not a priority sampler' do
let(:sampler) { double('sampler') }

context 'wraps sampler in a priority sampler' do
it_behaves_like 'new tracer' do
let(:options) do
{ sampler: be_a(Datadog::Tracing::Sampling::PrioritySampler) & have_attributes(
pre_sampler: sampler,
priority_sampler: be_a(Datadog::Tracing::Sampling::RuleSampler)
) }
end

it_behaves_like 'event publishing writer and priority sampler'
end
end
end
end
end

context 'disabled' do
let(:priority_sampling) { false }

it_behaves_like 'new tracer' do
let(:options) { { sampler: be_a(Datadog::Tracing::Sampling::RuleSampler) } }
end

context 'with :sampler' do
before do
allow(settings.tracing)
.to receive(:sampler)
.and_return(sampler)
end

let(:sampler) { double('sampler') }

it_behaves_like 'new tracer' do
let(:options) { { sampler: sampler } }
it_behaves_like 'event publishing writer and priority sampler'
end
end
it_behaves_like 'new tracer' do
let(:options) { { sampler: sampler } }
it_behaves_like 'event publishing writer and priority sampler'
end
end

Expand Down
19 changes: 2 additions & 17 deletions spec/datadog/tracing/configuration/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -404,21 +404,6 @@ def propagation_inject_style
end
end

describe '#priority_sampling' do
subject(:priority_sampling) { settings.tracing.priority_sampling }

it { is_expected.to be nil }
end

describe '#priority_sampling=' do
it 'updates the #priority_sampling setting' do
expect { settings.tracing.priority_sampling = true }
.to change { settings.tracing.priority_sampling }
.from(nil)
.to(true)
end
end

describe '#report_hostname' do
subject(:report_hostname) { settings.tracing.report_hostname }

Expand Down Expand Up @@ -668,7 +653,7 @@ def propagation_inject_style
end

describe '#writer_options=' do
let(:options) { { priority_sampling: true } }
let(:options) { { anything: double } }

it 'updates the #writer_options setting' do
expect { settings.tracing.test_mode.writer_options = options }
Expand Down Expand Up @@ -728,7 +713,7 @@ def propagation_inject_style
end

describe '#writer_options=' do
let(:options) { { priority_sampling: true } }
let(:options) { { anything: double } }

it 'updates the #writer_options setting' do
expect { settings.tracing.writer_options = options }
Expand Down
8 changes: 0 additions & 8 deletions spec/datadog/tracing/diagnostics/environment_logger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
'sampling_rules' => nil,
'integrations_loaded' => nil,
'partial_flushing_enabled' => false,
'priority_sampling_enabled' => false,
)
end
end
Expand Down Expand Up @@ -140,7 +139,6 @@
sampling_rules: nil,
integrations_loaded: nil,
partial_flushing_enabled: false,
priority_sampling_enabled: false,
)
end

Expand Down Expand Up @@ -216,12 +214,6 @@

it { is_expected.to include partial_flushing_enabled: true }
end

context 'with priority sampling enabled' do
before { expect(Datadog.configuration.tracing).to receive(:priority_sampling).and_return(true) }

it { is_expected.to include priority_sampling_enabled: true }
end
end
end

Expand Down
42 changes: 15 additions & 27 deletions spec/datadog/tracing/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,6 @@ def agent_receives_span_step3(previous_success)

# Test setup
c.tracing.sampler = custom_sampler if custom_sampler
c.tracing.priority_sampling = priority_sampling if priority_sampling
end

WebMock.enable!
Expand All @@ -412,7 +411,6 @@ def agent_receives_span_step3(previous_success)
let(:stats) { tracer.writer.stats }

let(:custom_sampler) { nil }
let(:priority_sampling) { false }

let(:trace_sampling_rate) { nil }
let(:json_rules) { JSON.dump(rules) if rules }
Expand Down Expand Up @@ -509,12 +507,12 @@ def agent_receives_span_step3(previous_success)

context 'by direct sampling' do
let(:custom_sampler) { no_sampler }
let(:priority_sampling) { false }

let(:no_sampler) do
Class.new do
def sample!(trace)
trace.reject!
trace.sampled = false
end
end.new
end
Expand Down Expand Up @@ -790,39 +788,31 @@ def sample!(trace)
end

let(:custom_sampler) do
instance_double(Datadog::Tracing::Sampling::Sampler, sample?: sample, sample!: sample, sample_rate: double)
instance_double(Datadog::Tracing::Sampling::Sampler, sample?: double, sample!: double, sample_rate: double)
end

context 'that accepts a span' do
let(:sample) { true }

before do
expect(custom_sampler).to receive(:sample!) do |trace|
trace.sampled = true
false
end
end

it 'flushes the span' do
tracer.trace('span') {}
try_wait_until { tracer.writer.stats[:traces_flushed] >= 1 }
end
end

it_behaves_like 'priority sampled', 1.0

# DEV: the `custom_sampler` is configured as a `pre_sampler` in the PrioritySampler.
# When `custom_sampler` returns `trace.sampled? == true`, the `post_sampler` is
# still consulted. This is unlikely to be the desired behaviour when a user configures
# `c.tracing.sampler = custom_sampler`.
# In practice, the `custom_sampler` can reject traces (`trace.sampled? == false`),
# but accepting them does not actually change the default sampler's behavior.
# Changing this is a breaking change.
it_behaves_like 'sampling decision', '-0' # This is incorrect. -4 (MANUAL) is the correct value.
it_behaves_like 'sampling decision', '-4' do
before do
pending(
'A custom sampler consults PrioritySampler#post_sampler for the final sampling decision. ' \
'This is incorrect, as a custom sampler should allow complete control of the sampling decision.'
)
context 'that rejects a span' do
before do
expect(custom_sampler).to receive(:sample!) do |trace|
trace.sampled = false
false
end
end
end

context 'that rejects a span' do
let(:sample) { false }
it 'drops trace at application side' do
expect(tracer.writer).to_not receive(:write)

Expand Down Expand Up @@ -896,7 +886,6 @@ def sample!(trace)

before do
Datadog.configure do |c|
c.tracing.priority_sampling = true
c.tracing.writer = writer
end

Expand Down Expand Up @@ -940,7 +929,6 @@ def sample!(trace)
Datadog.configure do |c|
c.agent.host = hostname
c.agent.port = port
c.tracing.priority_sampling = true
end
end

Expand Down

0 comments on commit 2d1e3c0

Please sign in to comment.