From 2d1e3c05389306e7c8b3dc1903972e5e78482a23 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Thu, 30 Nov 2023 17:34:16 -0800 Subject: [PATCH] Remove configuration for priority_sampling --- lib/datadog/core/telemetry/collector.rb | 1 - lib/datadog/tracing/component.rb | 65 +++++------------ lib/datadog/tracing/configuration/settings.rb | 6 -- .../tracing/diagnostics/environment_logger.rb | 6 -- sig/datadog/core/telemetry/collector.rbs | 2 +- .../core/configuration/components_spec.rb | 70 ++----------------- .../tracing/configuration/settings_spec.rb | 19 +---- .../diagnostics/environment_logger_spec.rb | 8 --- spec/datadog/tracing/integration_spec.rb | 42 ++++------- 9 files changed, 42 insertions(+), 177 deletions(-) diff --git a/lib/datadog/core/telemetry/collector.rb b/lib/datadog/core/telemetry/collector.rb index adfa4173539..083d7fda146 100644 --- a/lib/datadog/core/telemetry/collector.rb +++ b/lib/datadog/core/telemetry/collector.rb @@ -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' diff --git a/lib/datadog/tracing/component.rb b/lib/datadog/tracing/component.rb index 26e86b128f5..f20a525131b 100644 --- a/lib/datadog/tracing/component.rb +++ b/lib/datadog/tracing/component.rb @@ -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. diff --git a/lib/datadog/tracing/configuration/settings.rb b/lib/datadog/tracing/configuration/settings.rb index 558af6497ba..6fc57f7b07d 100644 --- a/lib/datadog/tracing/configuration/settings.rb +++ b/lib/datadog/tracing/configuration/settings.rb @@ -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 diff --git a/lib/datadog/tracing/diagnostics/environment_logger.rb b/lib/datadog/tracing/diagnostics/environment_logger.rb index d7d5fdf45e5..30bda2136fc 100644 --- a/lib/datadog/tracing/diagnostics/environment_logger.rb +++ b/lib/datadog/tracing/diagnostics/environment_logger.rb @@ -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 @@ -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 diff --git a/sig/datadog/core/telemetry/collector.rbs b/sig/datadog/core/telemetry/collector.rbs index a01d736d65e..03fc4fadf74 100644 --- a/sig/datadog/core/telemetry/collector.rbs +++ b/sig/datadog/core/telemetry/collector.rbs @@ -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 diff --git a/spec/datadog/core/configuration/components_spec.rb b/spec/datadog/core/configuration/components_spec.rb index 3053e0e3d1f..d080a9d7ae4 100644 --- a/spec/datadog/core/configuration/components_spec.rb +++ b/spec/datadog/core/configuration/components_spec.rb @@ -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 diff --git a/spec/datadog/tracing/configuration/settings_spec.rb b/spec/datadog/tracing/configuration/settings_spec.rb index 647b26322be..25ee0f5fdf7 100644 --- a/spec/datadog/tracing/configuration/settings_spec.rb +++ b/spec/datadog/tracing/configuration/settings_spec.rb @@ -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 } @@ -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 } @@ -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 } diff --git a/spec/datadog/tracing/diagnostics/environment_logger_spec.rb b/spec/datadog/tracing/diagnostics/environment_logger_spec.rb index 904eed91d47..2bdc49425c3 100644 --- a/spec/datadog/tracing/diagnostics/environment_logger_spec.rb +++ b/spec/datadog/tracing/diagnostics/environment_logger_spec.rb @@ -45,7 +45,6 @@ 'sampling_rules' => nil, 'integrations_loaded' => nil, 'partial_flushing_enabled' => false, - 'priority_sampling_enabled' => false, ) end end @@ -140,7 +139,6 @@ sampling_rules: nil, integrations_loaded: nil, partial_flushing_enabled: false, - priority_sampling_enabled: false, ) end @@ -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 diff --git a/spec/datadog/tracing/integration_spec.rb b/spec/datadog/tracing/integration_spec.rb index 1822f7eb18f..fe58332e8e2 100644 --- a/spec/datadog/tracing/integration_spec.rb +++ b/spec/datadog/tracing/integration_spec.rb @@ -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! @@ -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 } @@ -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 @@ -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) @@ -896,7 +886,6 @@ def sample!(trace) before do Datadog.configure do |c| - c.tracing.priority_sampling = true c.tracing.writer = writer end @@ -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