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.

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

Remove configuration for priority_sampling #3325

Merged
merged 1 commit into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
Loading