From 8b5983ea71df6040d7196a5f5458f7cf7f04f150 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Tue, 11 Jul 2023 12:24:47 +0200 Subject: [PATCH 01/22] When working on https://github.com/DataDog/dd-trace-rb/pull/2936 I noticed that calling `SafeDup.frozen_or_dup({a: :b})` on ruby 3.2 would break; because it tries to call `+(-){a: :b}` which doesn't work. I had to ensure the `SafeDup` module worked for all objects. I decided to extract that work into a separate PR to ease the work of reviewing. The method `frozen_or_dup` and `frozen_dup` for ruby 2.3 and forward only expect to receive String values. This could lead to future errors. By checking if the value is a String we can warranty the same behaviour for all kinds of objects Also, I added some extract refimients to be able to call `#dup` on `true, false, 1, 1.0` values. --- lib/datadog/core/utils/safe_dup.rb | 60 ++++- spec/datadog/core/utils/safe_dup_spec.rb | 266 ++++++++++++++++++++--- 2 files changed, 287 insertions(+), 39 deletions(-) diff --git a/lib/datadog/core/utils/safe_dup.rb b/lib/datadog/core/utils/safe_dup.rb index 33d20c11998..a31e9651d65 100644 --- a/lib/datadog/core/utils/safe_dup.rb +++ b/lib/datadog/core/utils/safe_dup.rb @@ -13,20 +13,70 @@ def dup end end + # Ensures #initialize can call true.dup safely + module RefineTrue + refine TrueClass do + def dup + self + end + end + end + + # Ensures #initialize can call false.dup safely + module RefineFalse + refine FalseClass do + def dup + self + end + end + end + + # Ensures #initialize can call 1.dup safely + module RefineInteger + refine Integer do + def dup + self + end + end + end + + # Ensures #initialize can call 1.0.dup safely + module RefineFloat + refine Float do + def dup + self + end + end + end + using RefineNil + using RefineTrue + using RefineFalse + using RefineInteger + using RefineFloat end # String#+@ was introduced in Ruby 2.3 if String.method_defined?(:+@) && String.method_defined?(:-@) def self.frozen_or_dup(v) - # If the string is not frozen, the +(-v) will: - # - first create a frozen deduplicated copy with -v - # - then it will dup it more efficiently with +v - v.frozen? ? v : +(-v) + case v + when String + # If the string is not frozen, the +(-v) will: + # - first create a frozen deduplicated copy with -v + # - then it will dup it more efficiently with +v + v.frozen? ? v : +(-v) + else + v.frozen? ? v : v.dup + end end def self.frozen_dup(v) - -v if v + case v + when String + -v if v + else + v.frozen? ? v : v.dup.freeze + end end else def self.frozen_or_dup(v) diff --git a/spec/datadog/core/utils/safe_dup_spec.rb b/spec/datadog/core/utils/safe_dup_spec.rb index d043cd00f00..34b16ef7663 100644 --- a/spec/datadog/core/utils/safe_dup_spec.rb +++ b/spec/datadog/core/utils/safe_dup_spec.rb @@ -1,62 +1,260 @@ require 'datadog/core/utils/safe_dup' RSpec.describe Datadog::Core::Utils::SafeDup do - describe '.frozen_or_dup' do - context 'when given a frozen string' do - it 'returns the original input' do - input = 'a_frozen_string'.freeze + context 'String' do + describe '.frozen_or_dup' do + context 'when given a frozen string' do + it 'returns the original input' do + input = 'a_frozen_string'.freeze - result = described_class.frozen_or_dup(input) + result = described_class.frozen_or_dup(input) - expect(input).to be_frozen + expect(input).to be_frozen - expect(result).to eq(input) - expect(result).to be(input) - expect(result).to be_frozen + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + + context 'when given a string' do + it 'returns a non frozen dupliacte' do + input = 'a_string' + + result = described_class.frozen_or_dup(input) + + expect(input).not_to be_frozen + + expect(result).to eq(input) + expect(result).not_to be(input) + expect(result).not_to be_frozen + end + end + end + + describe '.frozen_dup' do + context 'when given a frozen string' do + it 'returns the original input' do + input = 'a_frozen_string'.freeze + + result = described_class.frozen_dup(input) + + expect(input).to be_frozen + + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + + context 'when given a string' do + it 'returns a frozen duplicate' do + input = 'a_string' + + result = described_class.frozen_dup(input) + + expect(input).not_to be_frozen + + expect(result).to eq(input) + expect(result).not_to be(input) + expect(result).to be_frozen + end + end + end + end + + context 'Hash' do + describe '.frozen_or_dup' do + context 'when given a frozen hash' do + it 'returns the original input' do + input = { a: :b }.freeze + + result = described_class.frozen_or_dup(input) + + expect(input).to be_frozen + + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + + context 'when given a hash' do + it 'returns a non frozen dupliacte' do + input = { a: :b } + + result = described_class.frozen_or_dup(input) + + expect(input).not_to be_frozen + + expect(result).to eq(input) + expect(result).not_to be(input) + expect(result).not_to be_frozen + end end end - context 'when given a string' do - it 'returns a non frozen dupliacte' do - input = 'a_string' + describe '.frozen_dup' do + context 'when given a frozen hash' do + it 'returns the original input' do + input = { a: :b }.freeze + + result = described_class.frozen_dup(input) + + expect(input).to be_frozen + + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + + context 'when given a hash' do + it 'returns a frozen duplicate' do + input = { a: :b } + + result = described_class.frozen_dup(input) + + expect(input).not_to be_frozen + + expect(result).to eq(input) + expect(result).not_to be(input) + expect(result).to be_frozen + end + end + end + end + + context 'Boolean' do + before do + skip 'TrueClass and FalseClass are not frozen by default on ruby 2.1' if RUBY_VERSION < '2.2' + end + + describe '.frozen_or_dup' do + context 'when given a boolean' do + it 'returns the original input' do + input = true + + result = described_class.frozen_or_dup(input) + + expect(input).to be_frozen + + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + end + + describe '.frozen_dup' do + context 'when given a boolean' do + it 'returns the original input' do + input = true + + result = described_class.frozen_dup(input) + + expect(input).to be_frozen + + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + end + end + + context 'Array' do + describe '.frozen_or_dup' do + context 'when given a frozen array' do + it 'returns the original input' do + input = [1].freeze + + result = described_class.frozen_or_dup(input) + + expect(input).to be_frozen + + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + + context 'when given a string' do + it 'returns a non frozen array' do + input = [1] + + result = described_class.frozen_or_dup(input) + + expect(input).not_to be_frozen + + expect(result).to eq(input) + expect(result).not_to be(input) + expect(result).not_to be_frozen + end + end + end + + describe '.frozen_dup' do + context 'when given a frozen array' do + it 'returns the original input' do + input = [1].freeze + + result = described_class.frozen_dup(input) + + expect(input).to be_frozen + + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end + end + + context 'when given a array' do + it 'returns a frozen duplicate' do + input = [1] - result = described_class.frozen_or_dup(input) + result = described_class.frozen_dup(input) - expect(input).not_to be_frozen + expect(input).not_to be_frozen - expect(result).to eq(input) - expect(result).not_to be(input) - expect(result).not_to be_frozen + expect(result).to eq(input) + expect(result).not_to be(input) + expect(result).to be_frozen + end end end end - describe '.frozen_dup' do - context 'when given a frozen string' do - it 'returns the original input' do - input = 'a_frozen_string'.freeze + context 'Numeric' do + describe '.frozen_or_dup' do + context 'when given a numeric' do + it 'returns the original input' do + input = 1 - result = described_class.frozen_dup(input) + result = described_class.frozen_or_dup(input) - expect(input).to be_frozen + expect(input).to be_frozen - expect(result).to eq(input) - expect(result).to be(input) - expect(result).to be_frozen + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end end end - context 'when given a string' do - it 'returns a frozen duplicate' do - input = 'a_string' + describe '.frozen_dup' do + context 'when given a numeric' do + it 'returns the original input' do + input = 10.0 - result = described_class.frozen_dup(input) + result = described_class.frozen_dup(input) - expect(input).not_to be_frozen + expect(input).to be_frozen - expect(result).to eq(input) - expect(result).not_to be(input) - expect(result).to be_frozen + expect(result).to eq(input) + expect(result).to be(input) + expect(result).to be_frozen + end end end end From 6eb117628a266c9bdb7ebe28818448a5d9a8e753 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Wed, 12 Jul 2023 13:19:45 +0200 Subject: [PATCH 02/22] remove duplicate RefineNil module from codebase --- lib/datadog/tracing/span_operation.rb | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/lib/datadog/tracing/span_operation.rb b/lib/datadog/tracing/span_operation.rb index ce371337367..19369e0a62d 100644 --- a/lib/datadog/tracing/span_operation.rb +++ b/lib/datadog/tracing/span_operation.rb @@ -3,6 +3,7 @@ require_relative '../core/environment/identity' require_relative '../core/utils' require_relative '../core/utils/time' +require_relative '../core/utils/safe_dup' require_relative 'event' require_relative 'metadata' @@ -434,19 +435,6 @@ def message :parent, :span - if RUBY_VERSION < '2.2' # nil.dup only fails in Ruby 2.1 - # Ensures #initialize can call nil.dup safely - module RefineNil - refine NilClass do - def dup - self - end - end - end - - using RefineNil - end - # Create a Span from the operation which represents # the finalized measurement. We #dup here to prevent # mutation by reference; when this span is returned, From 08e62e4c6895b906d13756399a4db3ac0367caeb Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Wed, 12 Jul 2023 17:06:47 +0200 Subject: [PATCH 03/22] remove dup refinements --- lib/datadog/core/backport.rb | 20 +++++++++ lib/datadog/core/utils/safe_dup.rb | 63 +++------------------------ lib/datadog/tracing/span_operation.rb | 4 +- sig/datadog/core/backport.rbs | 4 ++ 4 files changed, 32 insertions(+), 59 deletions(-) diff --git a/lib/datadog/core/backport.rb b/lib/datadog/core/backport.rb index e80fcd8edab..6fc94b2d556 100644 --- a/lib/datadog/core/backport.rb +++ b/lib/datadog/core/backport.rb @@ -21,5 +21,25 @@ def string_delete_prefix(string, prefix) end end end + + # This module is used to provide features from Ruby 2.4+ to older Rubies + module BackportFrom24 + module_function + + if RUBY_VERSION < '2.4' + def dup(value) + case value + when NilClass, TrueClass, FalseClass, Integer, Float + value + else + value.dup + end + end + else + def dup(value) + value.dup + end + end + end end end diff --git a/lib/datadog/core/utils/safe_dup.rb b/lib/datadog/core/utils/safe_dup.rb index a31e9651d65..340218405e3 100644 --- a/lib/datadog/core/utils/safe_dup.rb +++ b/lib/datadog/core/utils/safe_dup.rb @@ -1,61 +1,10 @@ +require_relative '../backport' + module Datadog module Core module Utils # Helper methods for safer dup module SafeDup - if RUBY_VERSION < '2.2' # nil.dup only fails in Ruby 2.1 - # Ensures #initialize can call nil.dup safely - module RefineNil - refine NilClass do - def dup - self - end - end - end - - # Ensures #initialize can call true.dup safely - module RefineTrue - refine TrueClass do - def dup - self - end - end - end - - # Ensures #initialize can call false.dup safely - module RefineFalse - refine FalseClass do - def dup - self - end - end - end - - # Ensures #initialize can call 1.dup safely - module RefineInteger - refine Integer do - def dup - self - end - end - end - - # Ensures #initialize can call 1.0.dup safely - module RefineFloat - refine Float do - def dup - self - end - end - end - - using RefineNil - using RefineTrue - using RefineFalse - using RefineInteger - using RefineFloat - end - # String#+@ was introduced in Ruby 2.3 if String.method_defined?(:+@) && String.method_defined?(:-@) def self.frozen_or_dup(v) @@ -66,7 +15,7 @@ def self.frozen_or_dup(v) # - then it will dup it more efficiently with +v v.frozen? ? v : +(-v) else - v.frozen? ? v : v.dup + v.frozen? ? v : Core::BackportFrom24.dup(v) end end @@ -75,16 +24,16 @@ def self.frozen_dup(v) when String -v if v else - v.frozen? ? v : v.dup.freeze + v.frozen? ? v : Core::BackportFrom24.dup(v).freeze end end else def self.frozen_or_dup(v) - v.frozen? ? v : v.dup + v.frozen? ? v : Core::BackportFrom24.dup(v) end def self.frozen_dup(v) - v.frozen? ? v : v.dup.freeze + v.frozen? ? v : Core::BackportFrom24.dup(v).freeze end end end diff --git a/lib/datadog/tracing/span_operation.rb b/lib/datadog/tracing/span_operation.rb index 19369e0a62d..27f83d70015 100644 --- a/lib/datadog/tracing/span_operation.rb +++ b/lib/datadog/tracing/span_operation.rb @@ -445,8 +445,8 @@ def build_span duration: duration, end_time: @end_time, id: @id, - meta: meta.dup, - metrics: metrics.dup, + meta: Core::Utils::SafeDup.frozen_or_dup(meta), + metrics: Core::Utils::SafeDup.frozen_or_dup(metrics), parent_id: @parent_id, resource: @resource, service: @service, diff --git a/sig/datadog/core/backport.rbs b/sig/datadog/core/backport.rbs index daa910a53c5..9b3642fe192 100644 --- a/sig/datadog/core/backport.rbs +++ b/sig/datadog/core/backport.rbs @@ -3,5 +3,9 @@ module Datadog module BackportFrom25 def self?.string_delete_prefix: (::String string, ::String prefix) -> ::String end + + module BackportFrom24 + def self?.dup: (untyped value) -> untyped + end end end From 3bdb6bce55fcfc6242597d4b7f69d9b3be61f6d4 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Wed, 12 Jul 2023 17:00:51 -0700 Subject: [PATCH 04/22] Prevent telemetry requests from being traced --- lib/datadog/core/telemetry/http/transport.rb | 1 + .../tracing/contrib/http/circuit_breaker.rb | 22 ++------ lib/ddtrace/transport/ext.rb | 3 ++ .../tracing/contrib/http/circuit_breaker.rbs | 2 +- .../core/telemetry/http/transport_spec.rb | 7 +-- .../contrib/http/circuit_breaker_spec.rb | 50 +++++-------------- .../tracing/contrib/http/request_spec.rb | 24 +++++++++ spec/support/network_helpers.rb | 2 +- 8 files changed, 50 insertions(+), 61 deletions(-) diff --git a/lib/datadog/core/telemetry/http/transport.rb b/lib/datadog/core/telemetry/http/transport.rb index 42d46874019..e640e256e03 100644 --- a/lib/datadog/core/telemetry/http/transport.rb +++ b/lib/datadog/core/telemetry/http/transport.rb @@ -38,6 +38,7 @@ def request(request_type:, payload:) def headers(request_type:, api_version: Http::Ext::API_VERSION) { + Datadog::Transport::Ext::HTTP::HEADER_DD_INTERNAL_UNTRACED_REQUEST => '1', Http::Ext::HEADER_CONTENT_TYPE => Http::Ext::CONTENT_TYPE_APPLICATION_JSON, Http::Ext::HEADER_DD_TELEMETRY_API_VERSION => api_version, Http::Ext::HEADER_DD_TELEMETRY_REQUEST_TYPE => request_type, diff --git a/lib/datadog/tracing/contrib/http/circuit_breaker.rb b/lib/datadog/tracing/contrib/http/circuit_breaker.rb index 219ce01c91e..66a296a4bbb 100644 --- a/lib/datadog/tracing/contrib/http/circuit_breaker.rb +++ b/lib/datadog/tracing/contrib/http/circuit_breaker.rb @@ -10,7 +10,7 @@ module HTTP # For avoiding recursive traces. module CircuitBreaker def should_skip_tracing?(request) - return true if datadog_http_request?(request) || datadog_test_agent_http_request?(request) + return true if internal_request?(request) # we don't want a "shotgun" effect with two nested traces for one # logical get, and request is likely to call itself recursively @@ -23,23 +23,9 @@ def should_skip_tracing?(request) # We don't want to trace our own call to the API (they use net/http) # TODO: We don't want this kind of soft-check on HTTP requests. # Remove this when transport implements its own "skip tracing" mechanism. - def datadog_http_request?(request) - if request[Datadog::Transport::Ext::HTTP::HEADER_META_TRACER_VERSION] - true - else - false - end - end - - # Check if there is header present for not tracing this request. Necessary to prevent http requests - # used for checking if the APM Test Agent is running from being traced. - # TODO: Remove this when transport implements its own "skip tracing" mechanism. - def datadog_test_agent_http_request?(request) - if request['X-Datadog-Untraced-Request'] - true - else - false - end + def internal_request?(request) + !!(request[Datadog::Transport::Ext::HTTP::HEADER_META_TRACER_VERSION] || + request[Transport::Ext::HTTP::HEADER_DD_INTERNAL_UNTRACED_REQUEST]) end def should_skip_distributed_tracing?(client_config) diff --git a/lib/ddtrace/transport/ext.rb b/lib/ddtrace/transport/ext.rb index 94a67a65957..0300a03b5c2 100644 --- a/lib/ddtrace/transport/ext.rb +++ b/lib/ddtrace/transport/ext.rb @@ -23,6 +23,9 @@ module HTTP HEADER_META_LANG_VERSION = 'Datadog-Meta-Lang-Version' HEADER_META_LANG_INTERPRETER = 'Datadog-Meta-Lang-Interpreter' HEADER_META_TRACER_VERSION = 'Datadog-Meta-Tracer-Version' + + # Header that prevents the Net::HTTP integration from tracing internal trace requests. + HEADER_DD_INTERNAL_UNTRACED_REQUEST = 'DD-Internal-Untraced-Request' end # @public_api diff --git a/sig/datadog/tracing/contrib/http/circuit_breaker.rbs b/sig/datadog/tracing/contrib/http/circuit_breaker.rbs index 979eef26367..62ecc351a8a 100644 --- a/sig/datadog/tracing/contrib/http/circuit_breaker.rbs +++ b/sig/datadog/tracing/contrib/http/circuit_breaker.rbs @@ -4,7 +4,7 @@ module Datadog module HTTP module CircuitBreaker def should_skip_tracing?: (untyped request) -> (true | false) - def datadog_http_request?: (untyped request) -> (true | false) + def internal_request?: (untyped request) -> bool def should_skip_distributed_tracing?: (untyped client_config) -> untyped end diff --git a/spec/datadog/core/telemetry/http/transport_spec.rb b/spec/datadog/core/telemetry/http/transport_spec.rb index 449d35712d3..9418af84e3a 100644 --- a/spec/datadog/core/telemetry/http/transport_spec.rb +++ b/spec/datadog/core/telemetry/http/transport_spec.rb @@ -27,9 +27,10 @@ let(:env) { instance_double(Datadog::Core::Telemetry::Http::Env, body: payload, path: path) } let(:headers) do { - Datadog::Core::Telemetry::Http::Ext::HEADER_CONTENT_TYPE => 'application/json', - Datadog::Core::Telemetry::Http::Ext::HEADER_DD_TELEMETRY_API_VERSION => 'v1', - Datadog::Core::Telemetry::Http::Ext::HEADER_DD_TELEMETRY_REQUEST_TYPE => request_type, + 'Content-Type' => 'application/json', + 'DD-Telemetry-API-Version' => 'v1', + 'DD-Telemetry-Request-Type' => 'app-started', + 'DD-Internal-Untraced-Request' => '1', } end let(:hostname) { 'foo' } diff --git a/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb b/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb index 5fecd83a6b9..6c83773cf07 100644 --- a/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb +++ b/spec/datadog/tracing/contrib/http/circuit_breaker_spec.rb @@ -16,7 +16,7 @@ context 'given a normal request' do before do - allow(circuit_breaker).to receive(:datadog_http_request?) + allow(circuit_breaker).to receive(:internal_request?) .with(request) .and_return(false) @@ -28,7 +28,7 @@ context 'given a request that is a Datadog request' do before do - allow(circuit_breaker).to receive(:datadog_http_request?) + allow(circuit_breaker).to receive(:internal_request?) .with(request) .and_return(true) end @@ -45,7 +45,7 @@ end before do - allow(circuit_breaker).to receive(:datadog_http_request?) + allow(circuit_breaker).to receive(:internal_request?) .with(request) .and_return(false) @@ -56,8 +56,8 @@ end end - describe '#datadog_http_request?' do - subject(:datadog_http_request?) { circuit_breaker.datadog_http_request?(request) } + describe '#internal_request?' do + subject(:internal_request?) { circuit_breaker.internal_request?(request) } context 'given an HTTP request' do context "when the #{Datadog::Transport::Ext::HTTP::HEADER_META_TRACER_VERSION} header" do @@ -74,45 +74,19 @@ it { is_expected.to be false } end end - end - - context 'integration' do - context 'given a request from' do - let(:request) { @request } - - subject(:send_traces) do - # Capture the HTTP request directly from the transport, - # to make sure we have legitimate example. - expect(::Net::HTTP::Post).to receive(:new).and_wrap_original do |m, *args| - @request = m.call(*args) - end - - # The request may produce an error (because the transport cannot connect) - # but ignore this... we just need the request, not a successful response. - allow(Datadog.logger).to receive(:error) - - # Send a request, and make sure we captured it. - transport.send_traces(get_test_traces(1)) - - expect(@request).to be_a_kind_of(::Net::HTTP::Post) - end - context 'a Datadog Net::HTTP transport' do - before { expect(::Net::HTTP).to receive(:new) } - - let(:transport) { Datadog::Transport::HTTP.default } + context 'with the DD-Internal-Untraced-Request header' do + context 'is present' do + let(:request) { ::Net::HTTP::Post.new('/some/path', headers) } + let(:headers) { { 'DD-Internal-Untraced-Request' => 'anything' } } it { is_expected.to be true } end - context 'a Datadog UDS transport' do - let(:transport) do - Datadog::Transport::HTTP.default do |t| - t.adapter :unix, '/tmp/ddagent/trace.sock' - end - end + context 'is missing' do + let(:request) { ::Net::HTTP::Post.new('/some/path') } - it { is_expected.to be true } + it { is_expected.to be false } end end end diff --git a/spec/datadog/tracing/contrib/http/request_spec.rb b/spec/datadog/tracing/contrib/http/request_spec.rb index e7e0c7927b4..22765e3d580 100644 --- a/spec/datadog/tracing/contrib/http/request_spec.rb +++ b/spec/datadog/tracing/contrib/http/request_spec.rb @@ -223,6 +223,30 @@ end end + describe 'with an internal HTTP request' do + subject(:response) { client.get(path, headers) } + let(:headers) { { 'DD-Internal-Untraced-Request' => '1' } } + + before { stub_request(:get, "#{uri}#{path}") } + + it 'does not trace internal requests' do + response + expect(spans).to be_empty + end + + describe 'integration' do + let(:transport) { Datadog::Transport::HTTP.default } + + it 'does not create a span for the transport request' do + expect(Datadog::Tracing).to_not receive(:trace) + + transport.send_traces(get_test_traces(1)) + + expect(WebMock).to have_requested(:post, %r{/v0.4/traces}) + end + end + end + describe 'Net::HTTP object pin' do context 'when overriden with a different #service value' do subject(:response) { client.get(path) } diff --git a/spec/support/network_helpers.rb b/spec/support/network_helpers.rb index e5d547f57db..0961bafbeb8 100644 --- a/spec/support/network_helpers.rb +++ b/spec/support/network_helpers.rb @@ -43,7 +43,7 @@ def call_web_mock_function_with_agent_host_exclusions def check_availability_by_http_request(host, port) uri = URI("http://#{host}:#{port}/info") request = Net::HTTP::Get.new(uri) - request['X-Datadog-Untraced-Request'] = true + request[Datadog::Transport::Ext::HTTP::HEADER_DD_INTERNAL_UNTRACED_REQUEST] = true response = Net::HTTP.start(uri.hostname, uri.port) do |http| http.request(request) end From f4429a6d9501cc9e20985d92cdd71a0b30a40fa9 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Thu, 13 Jul 2023 09:27:29 +0200 Subject: [PATCH 05/22] Add comment to explain why we have a separate branch for String when using SafeDup --- lib/datadog/core/utils/safe_dup.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/datadog/core/utils/safe_dup.rb b/lib/datadog/core/utils/safe_dup.rb index 340218405e3..dd967237070 100644 --- a/lib/datadog/core/utils/safe_dup.rb +++ b/lib/datadog/core/utils/safe_dup.rb @@ -9,6 +9,11 @@ module SafeDup if String.method_defined?(:+@) && String.method_defined?(:-@) def self.frozen_or_dup(v) case v + # For the case of a String we use the methods +@ and -@. + # Those methods are only for String objects + # they are faster and chepaer on the memory side. + # Check the benchmark on + # https://github.com/DataDog/dd-trace-rb/pull/2704 when String # If the string is not frozen, the +(-v) will: # - first create a frozen deduplicated copy with -v @@ -21,6 +26,11 @@ def self.frozen_or_dup(v) def self.frozen_dup(v) case v + # For the case of a String we use the methods -@ + # That method are only for String objects + # they are faster and chepaer on the memory side. + # Check the benchmark on + # https://github.com/DataDog/dd-trace-rb/pull/2704 when String -v if v else From 1e39d95c5c1101def3148aedea483d187767e7eb Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Thu, 13 Jul 2023 12:46:19 +0200 Subject: [PATCH 06/22] improve spec descriptions --- spec/datadog/core/utils/safe_dup_spec.rb | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/spec/datadog/core/utils/safe_dup_spec.rb b/spec/datadog/core/utils/safe_dup_spec.rb index 34b16ef7663..6d7d4f1d8ab 100644 --- a/spec/datadog/core/utils/safe_dup_spec.rb +++ b/spec/datadog/core/utils/safe_dup_spec.rb @@ -11,14 +11,13 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end end context 'when given a string' do - it 'returns a non frozen dupliacte' do + it 'returns a non-frozen dupliacte' do input = 'a_string' result = described_class.frozen_or_dup(input) @@ -41,7 +40,6 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end @@ -73,14 +71,13 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end end context 'when given a hash' do - it 'returns a non frozen dupliacte' do + it 'returns a non-frozen dupliacte' do input = { a: :b } result = described_class.frozen_or_dup(input) @@ -103,7 +100,6 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end @@ -139,7 +135,6 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end @@ -155,7 +150,6 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end @@ -173,14 +167,13 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end end - context 'when given a string' do - it 'returns a non frozen array' do + context 'when given an array' do + it 'returns a non-frozen copy of that array' do input = [1] result = described_class.frozen_or_dup(input) @@ -203,13 +196,12 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end end - context 'when given a array' do + context 'when given an array' do it 'returns a frozen duplicate' do input = [1] @@ -235,7 +227,6 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end @@ -251,7 +242,6 @@ expect(input).to be_frozen - expect(result).to eq(input) expect(result).to be(input) expect(result).to be_frozen end From 368202ebf8c43d85bb5c8aacf2a239b3900bc477 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Thu, 13 Jul 2023 12:47:04 +0200 Subject: [PATCH 07/22] replace swicth case for if..else in SafeDup module --- lib/datadog/core/utils/safe_dup.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/datadog/core/utils/safe_dup.rb b/lib/datadog/core/utils/safe_dup.rb index dd967237070..75fc6bf85a8 100644 --- a/lib/datadog/core/utils/safe_dup.rb +++ b/lib/datadog/core/utils/safe_dup.rb @@ -8,13 +8,12 @@ module SafeDup # String#+@ was introduced in Ruby 2.3 if String.method_defined?(:+@) && String.method_defined?(:-@) def self.frozen_or_dup(v) - case v # For the case of a String we use the methods +@ and -@. # Those methods are only for String objects # they are faster and chepaer on the memory side. # Check the benchmark on # https://github.com/DataDog/dd-trace-rb/pull/2704 - when String + if v === String # If the string is not frozen, the +(-v) will: # - first create a frozen deduplicated copy with -v # - then it will dup it more efficiently with +v @@ -25,13 +24,12 @@ def self.frozen_or_dup(v) end def self.frozen_dup(v) - case v # For the case of a String we use the methods -@ # That method are only for String objects # they are faster and chepaer on the memory side. # Check the benchmark on # https://github.com/DataDog/dd-trace-rb/pull/2704 - when String + if v === String -v if v else v.frozen? ? v : Core::BackportFrom24.dup(v).freeze From 20f315bffcae18cf1da060e6a43e3b4022cfee2e Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Thu, 13 Jul 2023 12:48:30 +0200 Subject: [PATCH 08/22] remove module_function from Backport modules --- lib/datadog/core/backport.rb | 12 ++++-------- sig/datadog/core/backport.rbs | 4 ++-- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/datadog/core/backport.rb b/lib/datadog/core/backport.rb index 6fc94b2d556..8cc67cfbfb1 100644 --- a/lib/datadog/core/backport.rb +++ b/lib/datadog/core/backport.rb @@ -4,14 +4,12 @@ module Datadog module Core # This module is used to provide features from Ruby 2.5+ to older Rubies module BackportFrom25 - module_function - if ::String.method_defined?(:delete_prefix) - def string_delete_prefix(string, prefix) + def self.string_delete_prefix(string, prefix) string.delete_prefix(prefix) end else - def string_delete_prefix(string, prefix) + def self.string_delete_prefix(string, prefix) prefix = prefix.to_s if string.start_with?(prefix) string[prefix.length..-1] || raise('rbs-guard: String#[] is non-nil as `prefix` is guaranteed present') @@ -24,10 +22,8 @@ def string_delete_prefix(string, prefix) # This module is used to provide features from Ruby 2.4+ to older Rubies module BackportFrom24 - module_function - if RUBY_VERSION < '2.4' - def dup(value) + def self.dup(value) case value when NilClass, TrueClass, FalseClass, Integer, Float value @@ -36,7 +32,7 @@ def dup(value) end end else - def dup(value) + def self.dup(value) value.dup end end diff --git a/sig/datadog/core/backport.rbs b/sig/datadog/core/backport.rbs index 9b3642fe192..586e6fa58d3 100644 --- a/sig/datadog/core/backport.rbs +++ b/sig/datadog/core/backport.rbs @@ -1,11 +1,11 @@ module Datadog module Core module BackportFrom25 - def self?.string_delete_prefix: (::String string, ::String prefix) -> ::String + def self.string_delete_prefix: (::String string, ::String prefix) -> ::String end module BackportFrom24 - def self?.dup: (untyped value) -> untyped + def self.dup: (Object value) -> Object end end end From be7e099469d2345f0c668d06f9b2b83a24c06f43 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Thu, 13 Jul 2023 12:49:01 +0200 Subject: [PATCH 09/22] Add backport support for dup for Bignum and Fixnum --- lib/datadog/core/backport.rb | 4 +++- sig/datadog/core/backport.rbs | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/datadog/core/backport.rb b/lib/datadog/core/backport.rb index 8cc67cfbfb1..5ef4334a552 100644 --- a/lib/datadog/core/backport.rb +++ b/lib/datadog/core/backport.rb @@ -23,14 +23,16 @@ def self.string_delete_prefix(string, prefix) # This module is used to provide features from Ruby 2.4+ to older Rubies module BackportFrom24 if RUBY_VERSION < '2.4' + # rubocop:disable Lint/UnifiedInteger def self.dup(value) case value - when NilClass, TrueClass, FalseClass, Integer, Float + when NilClass, TrueClass, FalseClass, Integer, Float, Bignum, Fixnum value else value.dup end end + # rubocop:enable Lint/UnifiedInteger else def self.dup(value) value.dup diff --git a/sig/datadog/core/backport.rbs b/sig/datadog/core/backport.rbs index 586e6fa58d3..186d416e903 100644 --- a/sig/datadog/core/backport.rbs +++ b/sig/datadog/core/backport.rbs @@ -5,6 +5,9 @@ module Datadog end module BackportFrom24 + Fixnum: Integer + Bignum: Integer + def self.dup: (Object value) -> Object end end From d832464ad5cb29e07813123aa02a7256ea229427 Mon Sep 17 00:00:00 2001 From: Gustavo Caso Date: Thu, 13 Jul 2023 13:31:25 +0200 Subject: [PATCH 10/22] apply feedback --- lib/datadog/core/backport.rb | 4 +--- lib/datadog/core/utils/safe_dup.rb | 4 ++-- sig/datadog/core/backport.rbs | 3 --- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/datadog/core/backport.rb b/lib/datadog/core/backport.rb index 5ef4334a552..88a36f3a1ff 100644 --- a/lib/datadog/core/backport.rb +++ b/lib/datadog/core/backport.rb @@ -23,16 +23,14 @@ def self.string_delete_prefix(string, prefix) # This module is used to provide features from Ruby 2.4+ to older Rubies module BackportFrom24 if RUBY_VERSION < '2.4' - # rubocop:disable Lint/UnifiedInteger def self.dup(value) case value - when NilClass, TrueClass, FalseClass, Integer, Float, Bignum, Fixnum + when NilClass, TrueClass, FalseClass, Numeric value else value.dup end end - # rubocop:enable Lint/UnifiedInteger else def self.dup(value) value.dup diff --git a/lib/datadog/core/utils/safe_dup.rb b/lib/datadog/core/utils/safe_dup.rb index 75fc6bf85a8..5d74cdb2ac1 100644 --- a/lib/datadog/core/utils/safe_dup.rb +++ b/lib/datadog/core/utils/safe_dup.rb @@ -13,7 +13,7 @@ def self.frozen_or_dup(v) # they are faster and chepaer on the memory side. # Check the benchmark on # https://github.com/DataDog/dd-trace-rb/pull/2704 - if v === String + if v.is_a?(String) # If the string is not frozen, the +(-v) will: # - first create a frozen deduplicated copy with -v # - then it will dup it more efficiently with +v @@ -29,7 +29,7 @@ def self.frozen_dup(v) # they are faster and chepaer on the memory side. # Check the benchmark on # https://github.com/DataDog/dd-trace-rb/pull/2704 - if v === String + if v.is_a?(String) -v if v else v.frozen? ? v : Core::BackportFrom24.dup(v).freeze diff --git a/sig/datadog/core/backport.rbs b/sig/datadog/core/backport.rbs index 186d416e903..586e6fa58d3 100644 --- a/sig/datadog/core/backport.rbs +++ b/sig/datadog/core/backport.rbs @@ -5,9 +5,6 @@ module Datadog end module BackportFrom24 - Fixnum: Integer - Bignum: Integer - def self.dup: (Object value) -> Object end end From 39270801310f70bc4f2dd181fd3560ff7787aed0 Mon Sep 17 00:00:00 2001 From: Sarah Chen Date: Mon, 10 Jul 2023 14:53:43 -0400 Subject: [PATCH 11/22] add disk allocation variables in the environment in order to ensure the code works, even on a nearly full disk. --- .circleci/config.yml | 5 +++++ docker-compose.yml | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 40df2590fef..e997ef9be4e 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -78,6 +78,11 @@ test_containers: environment: - discovery.type=single-node - DISABLE_SECURITY_PLUGIN=true + # make sure it works on nearly full disk + - cluster.routing.allocation.disk.watermark.low=3gb + - cluster.routing.allocation.disk.watermark.high=2gb + - cluster.routing.allocation.disk.watermark.flood_stage=1gb + - cluster.routing.allocation.disk.threshold_enabled=false - &opensearch_port 9200 - &container_elasticsearch image: elasticsearch:8.1.3 diff --git a/docker-compose.yml b/docker-compose.yml index f67783f25cd..932fd6b2bec 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -468,6 +468,11 @@ services: environment: - discovery.type=single-node - DISABLE_SECURITY_PLUGIN=true + # make sure it works on nearly full disk + - cluster.routing.allocation.disk.watermark.low=3gb + - cluster.routing.allocation.disk.watermark.high=2gb + - cluster.routing.allocation.disk.watermark.flood_stage=1gb + - cluster.routing.allocation.disk.threshold_enabled=false ports: - 9201:9200 postgres: From d3be24ae91f436a59bedde7628dae3490fb862f6 Mon Sep 17 00:00:00 2001 From: Sarah Chen Date: Thu, 13 Jul 2023 14:04:16 -0400 Subject: [PATCH 12/22] change opensearch version to 1.3.6 which is more stable. add comment for context. --- .circleci/config.yml | 5 +++-- docker-compose.yml | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index e997ef9be4e..45a7afc19b6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -73,12 +73,13 @@ test_containers: - &mysql_port 3306 - &opensearch_host opensearch - &container_opensearch - image: opensearchproject/opensearch:2.8.0 + image: opensearchproject/opensearch:1.3.6 # Version 2.8.0 causes flaky error (https://github.com/opensearch-project/docker-images/issues/31). name: *opensearch_host environment: - discovery.type=single-node - DISABLE_SECURITY_PLUGIN=true - # make sure it works on nearly full disk + # Make sure it works on nearly full disk. + - cluster.routing.allocation.disk.threshold_enabled=true - cluster.routing.allocation.disk.watermark.low=3gb - cluster.routing.allocation.disk.watermark.high=2gb - cluster.routing.allocation.disk.watermark.flood_stage=1gb diff --git a/docker-compose.yml b/docker-compose.yml index 932fd6b2bec..17834ee2144 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -464,11 +464,12 @@ services: ports: - "127.0.0.1:${TEST_MYSQL_PORT}:3306" opensearch: - image: opensearchproject/opensearch:2.8.0 + image: opensearchproject/opensearch:1.3.6 # Version 2.8.0 causes flaky error (https://github.com/opensearch-project/docker-images/issues/31). environment: - discovery.type=single-node - DISABLE_SECURITY_PLUGIN=true - # make sure it works on nearly full disk + # Make sure it works on nearly full disk. + - cluster.routing.allocation.disk.threshold_enabled=true - cluster.routing.allocation.disk.watermark.low=3gb - cluster.routing.allocation.disk.watermark.high=2gb - cluster.routing.allocation.disk.watermark.flood_stage=1gb From 8089bb53542cec6d2970452bdab7d3150a312edd Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Thu, 13 Jul 2023 11:58:31 -0700 Subject: [PATCH 13/22] Clarify header value expectation --- lib/ddtrace/transport/ext.rb | 1 + spec/support/network_helpers.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ddtrace/transport/ext.rb b/lib/ddtrace/transport/ext.rb index 0300a03b5c2..5410e75e65b 100644 --- a/lib/ddtrace/transport/ext.rb +++ b/lib/ddtrace/transport/ext.rb @@ -25,6 +25,7 @@ module HTTP HEADER_META_TRACER_VERSION = 'Datadog-Meta-Tracer-Version' # Header that prevents the Net::HTTP integration from tracing internal trace requests. + # Set it to any value to skip tracing. HEADER_DD_INTERNAL_UNTRACED_REQUEST = 'DD-Internal-Untraced-Request' end diff --git a/spec/support/network_helpers.rb b/spec/support/network_helpers.rb index 0961bafbeb8..47f801a8ea1 100644 --- a/spec/support/network_helpers.rb +++ b/spec/support/network_helpers.rb @@ -43,7 +43,7 @@ def call_web_mock_function_with_agent_host_exclusions def check_availability_by_http_request(host, port) uri = URI("http://#{host}:#{port}/info") request = Net::HTTP::Get.new(uri) - request[Datadog::Transport::Ext::HTTP::HEADER_DD_INTERNAL_UNTRACED_REQUEST] = true + request[Datadog::Transport::Ext::HTTP::HEADER_DD_INTERNAL_UNTRACED_REQUEST] = '1' response = Net::HTTP.start(uri.hostname, uri.port) do |http| http.request(request) end From 490d98b374e6e94b35d6c3bbfe0d03ed2a74136b Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Thu, 13 Jul 2023 16:03:07 -0700 Subject: [PATCH 14/22] Internal:Fix environment variable for remote configuration polling interval --- lib/datadog/core/configuration/settings.rb | 3 ++- lib/datadog/core/remote/ext.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/datadog/core/configuration/settings.rb b/lib/datadog/core/configuration/settings.rb index 3edffa85348..acd932a6f6f 100644 --- a/lib/datadog/core/configuration/settings.rb +++ b/lib/datadog/core/configuration/settings.rb @@ -541,8 +541,9 @@ def initialize(*_) # Tune remote configuration polling interval. # - # @default `DD_REMOTE_CONFIGURATION_POLL_INTERVAL_SECONDS` environment variable, otherwise `5.0` seconds. + # @default `DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS` environment variable, otherwise `5.0` seconds. # @return [Float] + # @!visibility private option :poll_interval_seconds do |o| o.default { env_to_float(Core::Remote::Ext::ENV_POLL_INTERVAL_SECONDS, 5.0) } end diff --git a/lib/datadog/core/remote/ext.rb b/lib/datadog/core/remote/ext.rb index 833aaec1362..a9bd38f744f 100644 --- a/lib/datadog/core/remote/ext.rb +++ b/lib/datadog/core/remote/ext.rb @@ -5,7 +5,7 @@ module Core module Remote module Ext ENV_ENABLED = 'DD_REMOTE_CONFIGURATION_ENABLED' - ENV_POLL_INTERVAL_SECONDS = 'DD_REMOTE_CONFIGURATION_POLL_INTERVAL_SECONDS' + ENV_POLL_INTERVAL_SECONDS = 'DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS' end end end From f7e8e53a7ae042d2a54c8cf14eedceb4f6d0f194 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Thu, 13 Jul 2023 19:15:29 -0700 Subject: [PATCH 15/22] Add sampling configuration by DD_TRACE_SAMPLING_RULES --- docs/GettingStarted.md | 1 + lib/datadog/tracing/component.rb | 14 +++ lib/datadog/tracing/configuration/ext.rb | 1 + lib/datadog/tracing/configuration/settings.rb | 16 ++++ lib/datadog/tracing/sampling/rule_sampler.rb | 27 ++++++ sig/datadog/tracing/configuration/ext.rbs | 1 + .../core/configuration/components_spec.rb | 20 ++++ .../tracing/configuration/settings_spec.rb | 18 ++++ spec/datadog/tracing/integration_spec.rb | 48 ++++++++++ .../tracing/sampling/rule_sampler_spec.rb | 94 +++++++++++++++++++ 10 files changed, 240 insertions(+) diff --git a/docs/GettingStarted.md b/docs/GettingStarted.md index e18cafbd2c9..203ea9e2d3f 100644 --- a/docs/GettingStarted.md +++ b/docs/GettingStarted.md @@ -2239,6 +2239,7 @@ For example, if `tracing.sampling.default_rate` is configured by [Remote Configu | `tracing.sampler` | | `nil` | Advanced usage only. Sets a custom `Datadog::Tracing::Sampling::Sampler` instance. If provided, the tracer will use this sampler to determine sampling behavior. See [Application-side sampling](#application-side-sampling) for details. | | `tracing.sampling.default_rate` | `DD_TRACE_SAMPLE_RATE` | `nil` | Sets the trace sampling rate between `0.0` (0%) and `1.0` (100%). See [Application-side sampling](#application-side-sampling) for details. | | `tracing.sampling.rate_limit` | `DD_TRACE_RATE_LIMIT` | `100` (per second) | Sets a maximum number of traces per second to sample. Set a rate limit to avoid the ingestion volume overages in the case of traffic spikes. | +| `tracing.sampling.rules` | `DD_TRACE_SAMPLING_RULES` | `nil` | Sets trace-level sampling rules, matching against the local root span. The format is a `String` with JSON, containing an Array of Objects. Each Object must have a float attribute `sample_rate` (between 0.0 and 1.0, inclusive), and optionally `name` and `service` string attributes. `name` and `service` control to which traces this sampling rule applies; if both are absent, then this rule applies to all traces. Rules are evaluted in order of declartion in the array; only the first to match is applied. If none apply, then `tracing.sampling.default_rate` is applied. | | `tracing.sampling.span_rules` | `DD_SPAN_SAMPLING_RULES`,`ENV_SPAN_SAMPLING_RULES_FILE` | `nil` | Sets [Single Span Sampling](#single-span-sampling) rules. These rules allow you to keep spans even when their respective traces are dropped. | | `tracing.trace_id_128_bit_generation_enabled` | `DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED` | `false` | `true` to generate 128 bits trace ID and `false` to generate 64 bits trace ID | | `tracing.report_hostname` | `DD_TRACE_REPORT_HOSTNAME` | `false` | Adds hostname tag to traces. | diff --git a/lib/datadog/tracing/component.rb b/lib/datadog/tracing/component.rb index 74401954d91..596270d6e78 100644 --- a/lib/datadog/tracing/component.rb +++ b/lib/datadog/tracing/component.rb @@ -62,6 +62,20 @@ def build_sampler(settings) else ensure_priority_sampling(sampler, settings) end + elsif (rules = settings.tracing.sampling.rules) + Tracing::Sampling::PrioritySampler.new( + base_sampler: Tracing::Sampling::AllSampler.new, + post_sampler: Tracing::Sampling::RuleSampler.parse( + rules, + settings.tracing.sampling.rate_limit, + settings.tracing.sampling.default_rate + ) || + # 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 + ) + ) elsif settings.tracing.priority_sampling == false Tracing::Sampling::RuleSampler.new( rate_limit: settings.tracing.sampling.rate_limit, diff --git a/lib/datadog/tracing/configuration/ext.rb b/lib/datadog/tracing/configuration/ext.rb index e46e77203ab..283035b7f9f 100644 --- a/lib/datadog/tracing/configuration/ext.rb +++ b/lib/datadog/tracing/configuration/ext.rb @@ -72,6 +72,7 @@ module NET module Sampling ENV_SAMPLE_RATE = 'DD_TRACE_SAMPLE_RATE' ENV_RATE_LIMIT = 'DD_TRACE_RATE_LIMIT' + ENV_RULES = 'DD_TRACE_SAMPLING_RULES' # @public_api module Span diff --git a/lib/datadog/tracing/configuration/settings.rb b/lib/datadog/tracing/configuration/settings.rb index 09e4fdba839..14bc7382133 100644 --- a/lib/datadog/tracing/configuration/settings.rb +++ b/lib/datadog/tracing/configuration/settings.rb @@ -277,6 +277,22 @@ def self.extended(base) o.default { env_to_float(Tracing::Configuration::Ext::Sampling::ENV_RATE_LIMIT, 100) } end + # Trace sampling rules. + # These rules control whether a trace is kept or dropped by the tracer. + # + # The `rules` format is a String with a JSON array of objects: + # Each object must have a `sample_rate`, and the `name` and `service` fields + # are optional. The `sample_rate` value must be between 0.0 and 1.0 (inclusive). + # `name` and `service` are Strings that allow the `sample_rate` to be applied only + # to traces matching the `name` and `service`. + # + # @default `DD_TRACE_SAMPLING_RULES` environment variable. Otherwise `nil`. + # @return [String,nil] + # @public_api + option :rules do |o| + o.default { ENV.fetch(Configuration::Ext::Sampling::ENV_RULES, nil) } + end + # Single span sampling rules. # These rules allow a span to be kept when its encompassing trace is dropped. # diff --git a/lib/datadog/tracing/sampling/rule_sampler.rb b/lib/datadog/tracing/sampling/rule_sampler.rb index 94ed709501d..4edd9890e7d 100644 --- a/lib/datadog/tracing/sampling/rule_sampler.rb +++ b/lib/datadog/tracing/sampling/rule_sampler.rb @@ -48,6 +48,33 @@ def initialize( end end + def self.parse(rules, rate_limit, default_sample_rate) + rules = JSON.parse(rules).map do |rule| + sample_rate = rule['sample_rate'] + + begin + sample_rate = Float(sample_rate) + rescue + raise "Rule '#{rule.inspect}' does not contain a float property `sample_rate`" + end + + kwargs = { + name: rule['name'], + service: rule['service'], + sample_rate: sample_rate, + }.compact + + SimpleRule.new(**kwargs) + end + + new(rules, rate_limit: rate_limit, default_sample_rate: default_sample_rate) + rescue => e + Datadog.logger.error do + "Could not parse trace sampling rules '#{rules}': #{e.class.name} #{e.message} at #{Array(e.backtrace).first}" + end + nil + end + # /RuleSampler's components (it's rate limiter, for example) are # not be guaranteed to be size-effect free. # It is not possible to guarantee that a call to {#sample?} will diff --git a/sig/datadog/tracing/configuration/ext.rbs b/sig/datadog/tracing/configuration/ext.rbs index 29b6215e1ad..522b06350da 100644 --- a/sig/datadog/tracing/configuration/ext.rbs +++ b/sig/datadog/tracing/configuration/ext.rbs @@ -44,6 +44,7 @@ module Datadog ENV_REPORT_HOSTNAME: "DD_TRACE_REPORT_HOSTNAME" end module Sampling + ENV_RULES: String ENV_SAMPLE_RATE: "DD_TRACE_SAMPLE_RATE" ENV_RATE_LIMIT: "DD_TRACE_RATE_LIMIT" diff --git a/spec/datadog/core/configuration/components_spec.rb b/spec/datadog/core/configuration/components_spec.rb index 1d1c88b14ce..2c4c297c71a 100644 --- a/spec/datadog/core/configuration/components_spec.rb +++ b/spec/datadog/core/configuration/components_spec.rb @@ -634,6 +634,26 @@ end end + context 'with sampling.rules' do + before { allow(settings.tracing.sampling).to receive(:rules).and_return(rules) } + + context 'with rules' do + let(:rules) { '[{"sample_rate":"0.123"}]' } + + it_behaves_like 'new tracer' do + let(:sampler) do + lambda do |sampler| + expect(sampler).to be_a(Datadog::Tracing::Sampling::PrioritySampler) + expect(sampler.pre_sampler).to be_a(Datadog::Tracing::Sampling::AllSampler) + + expect(sampler.priority_sampler.rules).to have(1).item + expect(sampler.priority_sampler.rules[0].sampler.sample_rate).to eq(0.123) + end + end + end + end + end + context 'with sampling.span_rules' do before { allow(settings.tracing.sampling).to receive(:span_rules).and_return(rules) } diff --git a/spec/datadog/tracing/configuration/settings_spec.rb b/spec/datadog/tracing/configuration/settings_spec.rb index d8952224b5e..c8ad79f59dd 100644 --- a/spec/datadog/tracing/configuration/settings_spec.rb +++ b/spec/datadog/tracing/configuration/settings_spec.rb @@ -457,6 +457,24 @@ def propagation_inject_style end end + describe '#rules' do + subject(:rules) { settings.tracing.sampling.rules } + + context 'default' do + it { is_expected.to be_nil } + end + + context 'when ENV is provided' do + around do |example| + ClimateControl.modify('DD_TRACE_SAMPLING_RULES' => '[{"sample_rate":0.2}]') do + example.run + end + end + + it { is_expected.to eq('[{"sample_rate":0.2}]') } + end + end + describe '#span_rules' do subject(:rules) { settings.tracing.sampling.span_rules } diff --git a/spec/datadog/tracing/integration_spec.rb b/spec/datadog/tracing/integration_spec.rb index ba0aae3f3a1..e0f07fdebe2 100644 --- a/spec/datadog/tracing/integration_spec.rb +++ b/spec/datadog/tracing/integration_spec.rb @@ -285,6 +285,17 @@ def agent_receives_span_step3(previous_success) it_behaves_like 'sampling decision', nil end + shared_context 'DD_TRACE_SAMPLING_RULES configuration' do + let(:sampler) { nil } + let(:rules_json) { [rule].to_json } + + around do |example| + ClimateControl.modify('DD_TRACE_SAMPLING_RULES' => rules_json) do + example.run + end + end + end + context 'with rule' do let(:rule_sampler) { Datadog::Tracing::Sampling::RuleSampler.new([rule], **rule_sampler_opt) } let(:rule_sampler_opt) { {} } @@ -298,6 +309,18 @@ def agent_receives_span_step3(previous_success) it_behaves_like 'rate limit metric', 1.0 it_behaves_like 'sampling decision', '-3' + context 'set through DD_TRACE_SAMPLING_RULES environment variable' do + include_context 'DD_TRACE_SAMPLING_RULES configuration' do + let(:rule) { { name: 'my.op', sample_rate: 1.0 } } + end + + it_behaves_like 'flushed trace' + it_behaves_like 'priority sampled', Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP + it_behaves_like 'rule sampling rate metric', 1.0 + it_behaves_like 'rate limit metric', 1.0 + it_behaves_like 'sampling decision', '-3' + end + context 'with low sample rate' do let(:rule) { Datadog::Tracing::Sampling::SimpleRule.new(sample_rate: Float::MIN) } @@ -306,6 +329,18 @@ def agent_receives_span_step3(previous_success) it_behaves_like 'rule sampling rate metric', Float::MIN it_behaves_like 'rate limit metric', nil # Rate limiter is never reached, thus has no value to provide it_behaves_like 'sampling decision', nil + + context 'set through DD_TRACE_SAMPLING_RULES environment variable' do + include_context 'DD_TRACE_SAMPLING_RULES configuration' do + let(:rule) { { sample_rate: Float::MIN } } + end + + it_behaves_like 'flushed trace' + it_behaves_like 'priority sampled', Datadog::Tracing::Sampling::Ext::Priority::USER_REJECT + it_behaves_like 'rule sampling rate metric', Float::MIN + it_behaves_like 'rate limit metric', nil # Rate limiter is never reached, thus has no value to provide + it_behaves_like 'sampling decision', nil + end end context 'rate limited' do @@ -328,6 +363,19 @@ def agent_receives_span_step3(previous_success) it_behaves_like 'rule sampling rate metric', nil it_behaves_like 'rate limit metric', nil it_behaves_like 'sampling decision', '-0' + + context 'set through DD_TRACE_SAMPLING_RULES environment variable' do + include_context 'DD_TRACE_SAMPLING_RULES configuration' do + let(:rule) { { name: 'not.my.op' } } + + it_behaves_like 'flushed trace' + # The PrioritySampler was responsible for the sampling decision, not the Rule Sampler. + it_behaves_like 'priority sampled', Datadog::Tracing::Sampling::Ext::Priority::AUTO_KEEP + it_behaves_like 'rule sampling rate metric', nil + it_behaves_like 'rate limit metric', nil + it_behaves_like 'sampling decision', '-0' + end + end end end end diff --git a/spec/datadog/tracing/sampling/rule_sampler_spec.rb b/spec/datadog/tracing/sampling/rule_sampler_spec.rb index 01bc4e6ba0c..b42977389ae 100644 --- a/spec/datadog/tracing/sampling/rule_sampler_spec.rb +++ b/spec/datadog/tracing/sampling/rule_sampler_spec.rb @@ -77,6 +77,100 @@ end end + describe 'parse' do + subject(:parse) { described_class.parse(rules.to_json, rate_limit, default_sample_rate) } + let(:rules) { [rule] } + let(:rate_limit) { nil } + let(:default_sample_rate) { nil } + + let(:actual_rule) do + subject + expect(parse.rules).to have(1).item + parse.rules[0] + end + + let(:actual_rules) do + subject + parse.rules + end + + context 'with sample_rate' do + let(:rule) { { sample_rate: 0.1 } } + + it 'parses as a match any' do + expect(actual_rule.matcher.name).to eq(Datadog::Tracing::Sampling::SimpleMatcher::MATCH_ALL) + expect(actual_rule.matcher.service).to eq(Datadog::Tracing::Sampling::SimpleMatcher::MATCH_ALL) + expect(actual_rule.sampler.sample_rate).to eq(0.1) + end + + context 'and name' do + let(:rule) { super().merge(name: 'test-name') } + + it 'parses matching any service' do + expect(actual_rule.matcher.name).to eq('test-name') + expect(actual_rule.matcher.service).to eq(Datadog::Tracing::Sampling::SimpleMatcher::MATCH_ALL) + expect(actual_rule.sampler.sample_rate).to eq(0.1) + end + end + + context 'and service' do + let(:rule) { super().merge(service: 'test-service') } + + it 'parses matching any name' do + expect(actual_rule.matcher.name).to eq(Datadog::Tracing::Sampling::SimpleMatcher::MATCH_ALL) + expect(actual_rule.matcher.service).to eq('test-service') + expect(actual_rule.sampler.sample_rate).to eq(0.1) + end + end + + context 'with multiple rules' do + let(:rules) { [{ sample_rate: 0.1 }, { sample_rate: 0.2 }] } + + it 'parses all rules in order' do + expect(actual_rules).to have(2).item + expect(actual_rules[0].sampler.sample_rate).to eq(0.1) + expect(actual_rules[1].sampler.sample_rate).to eq(0.2) + end + end + end + + context 'with a non-float sample_rate' do + let(:rule) { { sample_rate: 'oops' } } + + it 'does not accept rule with a non-float sample_rate' do + expect(Datadog.logger).to receive(:error) + is_expected.to be_nil + end + end + + context 'without a sample_rate' do + let(:rule) { { name: 'test' } } + + it 'does not accept rule missing the mandatory sample_rate' do + expect(Datadog.logger).to receive(:error) + is_expected.to be_nil + end + + context 'with multiple rules' do + let(:rules) { [{ sample_rate: 0.1 }, { name: 'test' }] } + + it 'rejects all rules if one is missing the mandatory sample_rate' do + expect(Datadog.logger).to receive(:error) + is_expected.to be_nil + end + end + end + + context 'without a valid JSON array' do + let(:rules) { 'not a json array' } + + it 'returns nil in case of parsing error' do + expect(Datadog.logger).to receive(:error) + is_expected.to be_nil + end + end + end + shared_context 'matching rule' do let(:rules) { [rule] } let(:rule) { instance_double(Datadog::Tracing::Sampling::Rule) } From a6c23935e0707910d01799ecf5aeb75c2b63df71 Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Fri, 14 Jul 2023 10:44:40 +0100 Subject: [PATCH 16/22] Tweak versions of google-protobuf used for testing in CI **What does this PR do?**: This PR tweaks the versions of google-protobuf used for testing to make sure to avoid the latest 3.24.0.rc.1 being a bit problematic for some legacy Rubies. This caused some failures in our CI: https://app.circleci.com/pipelines/github/DataDog/dd-trace-rb/10871/workflows/90c509f6-a12a-441a-8e1d-ad0b1567efed/jobs/405709 And has been reported upstream: https://github.com/protocolbuffers/protobuf/issues/13307 **Motivation**: Avoid problematic google-protobuf versions in CI. **Additional Notes**: The google-protobuf gem has some history of having incorrect packaging, which is why we already had a bunch of versions excluded (if you look at the PR diffs). One more for the pile! Google-protobuf is a dependency needed for the old profiler bits, and the profiler includes some help/detection to guide customers around these packaging issues when they are detected. Historically, not a lot of customers reached out with issues regarding google-protobuf, so I think it's mostly a problem for our CI which is more complex since it needs to run on a bunch of different rubies/platforms/etc. **How to test the change?**: Validate that CI is green! --- Gemfile | 5 ++--- integration/apps/rack/Gemfile | 3 ++- integration/apps/rails-five/Gemfile | 1 + integration/apps/rails-six/Gemfile | 3 ++- integration/apps/sinatra2-classic/Gemfile | 3 ++- integration/apps/sinatra2-modular/Gemfile | 3 ++- 6 files changed, 11 insertions(+), 7 deletions(-) diff --git a/Gemfile b/Gemfile index ae9ba592bcc..cc56608b026 100644 --- a/Gemfile +++ b/Gemfile @@ -97,12 +97,11 @@ gem 'dogstatsd-ruby', '>= 3.3.0', '!= 5.0.0', '!= 5.0.1', '!= 5.1.0' gem 'opentracing', '>= 0.4.1' # Profiler optional dependencies -# NOTE: We're excluding versions 3.7.0 and 3.7.1 for the reasons documented in #1424 and the big comment in -# lib/datadog/profiling.rb: it breaks for some older rubies in CI without BUNDLE_FORCE_RUBY_PLATFORM=true. +# NOTE: We're excluding versions 3.7.0 and 3.7.1 for the reasons documented in #1424. # Since most of our customers won't have BUNDLE_FORCE_RUBY_PLATFORM=true, it's not something we want to add # to our CI, so we just shortcut and exclude specific versions that were affecting our CI. if RUBY_PLATFORM != 'java' - if RUBY_VERSION >= '2.5.0' # Bundler 1.x fails to find that versions >= 3.8.0 are not compatible because of binary gems + if RUBY_VERSION >= '2.7.0' # Bundler 1.x fails to find that versions >= 3.8.0 are not compatible because of binary gems gem 'google-protobuf', ['~> 3.0', '!= 3.7.0', '!= 3.7.1'] elsif RUBY_VERSION >= '2.3.0' gem 'google-protobuf', ['~> 3.0', '!= 3.7.0', '!= 3.7.1', '< 3.19.2'] diff --git a/integration/apps/rack/Gemfile b/integration/apps/rack/Gemfile index 25150b545d4..4473c781fb1 100644 --- a/integration/apps/rack/Gemfile +++ b/integration/apps/rack/Gemfile @@ -32,7 +32,8 @@ google_protobuf_versions = [ '!= 3.7.0.rc.3', '!= 3.7.0', '!= 3.7.1', - '!= 3.8.0.rc.1' + '!= 3.8.0.rc.1', + '!= 3.24.0.rc.1', ] if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.4') gem 'google-protobuf', *google_protobuf_versions diff --git a/integration/apps/rails-five/Gemfile b/integration/apps/rails-five/Gemfile index 9150ca071f3..4b71c54ff1d 100644 --- a/integration/apps/rails-five/Gemfile +++ b/integration/apps/rails-five/Gemfile @@ -13,6 +13,7 @@ google_protobuf_versions = [ '!= 3.8.0.rc.1', '!= 3.20.0.rc.1', '!= 3.20.0.rc.2', + '!= 3.24.0.rc.1', ] rails_version = ['~> 5.2', '>= 5.2.6'] diff --git a/integration/apps/rails-six/Gemfile b/integration/apps/rails-six/Gemfile index cccbe5f64f0..bcaa7a8f782 100644 --- a/integration/apps/rails-six/Gemfile +++ b/integration/apps/rails-six/Gemfile @@ -8,7 +8,8 @@ google_protobuf_versions = [ '!= 3.7.0.rc.3', '!= 3.7.0', '!= 3.7.1', - '!= 3.8.0.rc.1' + '!= 3.8.0.rc.1', + '!= 3.24.0.rc.1', ] rails_version = ['~> 6.1'] diff --git a/integration/apps/sinatra2-classic/Gemfile b/integration/apps/sinatra2-classic/Gemfile index 26e430925d6..aac441950e9 100644 --- a/integration/apps/sinatra2-classic/Gemfile +++ b/integration/apps/sinatra2-classic/Gemfile @@ -17,7 +17,8 @@ google_protobuf_versions = [ '!= 3.7.0.rc.3', '!= 3.7.0', '!= 3.7.1', - '!= 3.8.0.rc.1' + '!= 3.8.0.rc.1', + '!= 3.24.0.rc.1', ] if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.4') gem 'google-protobuf', *google_protobuf_versions diff --git a/integration/apps/sinatra2-modular/Gemfile b/integration/apps/sinatra2-modular/Gemfile index 2752a28be96..9e87d693e41 100644 --- a/integration/apps/sinatra2-modular/Gemfile +++ b/integration/apps/sinatra2-modular/Gemfile @@ -18,7 +18,8 @@ google_protobuf_versions = [ '!= 3.7.0.rc.3', '!= 3.7.0', '!= 3.7.1', - '!= 3.8.0.rc.1' + '!= 3.8.0.rc.1', + '!= 3.24.0.rc.1', ] if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.4') gem 'google-protobuf', *google_protobuf_versions From ec5cac0f9547e28ddcb26ab95e72fa1299c4b9f9 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Fri, 14 Jul 2023 11:19:53 -0700 Subject: [PATCH 17/22] Doc:Add Ruby 3.2 support to our compatibility matrix --- docs/GettingStarted.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/GettingStarted.md b/docs/GettingStarted.md index e18cafbd2c9..2cfb0083cdb 100644 --- a/docs/GettingStarted.md +++ b/docs/GettingStarted.md @@ -122,7 +122,8 @@ To contribute, check out the [contribution guidelines][contribution docs] and [d | Type | Documentation | Version | Support type | Gem version support | | ----- | -------------------------- | ----- | ------------------------------------ | ------------------- | -| MRI | https://www.ruby-lang.org/ | 3.1 | Full | Latest | +| MRI | https://www.ruby-lang.org/ | 3.2 | Full | Latest | +| | | 3.1 | Full | Latest | | | | 3.0 | Full | Latest | | | | 2.7 | Full | Latest | | | | 2.6 | Full | Latest | @@ -1714,7 +1715,7 @@ end | 2.4 | | 4.2.8 - 5.2 | | 2.5 | | 4.2.8 - 6.1 | | 2.6 - 2.7 | 9.2 | 5.0 - 6.1 | -| 3.0 | | 6.1 | +| 3.0 - 3.2 | | 6.1 | ### Rake From fe971f4b1cf6fcb0ee398cc945916553eac23e01 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Fri, 14 Jul 2023 14:15:50 -0700 Subject: [PATCH 18/22] Improve private language --- lib/datadog/core/configuration/settings.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/datadog/core/configuration/settings.rb b/lib/datadog/core/configuration/settings.rb index acd932a6f6f..b1eb701a0f5 100644 --- a/lib/datadog/core/configuration/settings.rb +++ b/lib/datadog/core/configuration/settings.rb @@ -540,6 +540,7 @@ def initialize(*_) end # Tune remote configuration polling interval. + # This is a private setting. Do not use outside of Datadog. It might change at any point in time. # # @default `DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS` environment variable, otherwise `5.0` seconds. # @return [Float] From e16db3abe004a66014a0809af1c70cff09aa4d1d Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Fri, 14 Jul 2023 14:21:55 -0700 Subject: [PATCH 19/22] Apply comments --- lib/datadog/tracing/component.rb | 23 +++++++++++-------- lib/datadog/tracing/sampling/rule_sampler.rb | 4 ++-- .../tracing/sampling/rule_sampler_spec.rb | 6 ++--- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/datadog/tracing/component.rb b/lib/datadog/tracing/component.rb index 596270d6e78..0cb993ab1d2 100644 --- a/lib/datadog/tracing/component.rb +++ b/lib/datadog/tracing/component.rb @@ -63,18 +63,21 @@ def build_sampler(settings) ensure_priority_sampling(sampler, settings) end elsif (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: Tracing::Sampling::RuleSampler.parse( - rules, - settings.tracing.sampling.rate_limit, - settings.tracing.sampling.default_rate - ) || - # 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 - ) + post_sampler: post_sampler ) elsif settings.tracing.priority_sampling == false Tracing::Sampling::RuleSampler.new( diff --git a/lib/datadog/tracing/sampling/rule_sampler.rb b/lib/datadog/tracing/sampling/rule_sampler.rb index 4edd9890e7d..698fec3b54d 100644 --- a/lib/datadog/tracing/sampling/rule_sampler.rb +++ b/lib/datadog/tracing/sampling/rule_sampler.rb @@ -49,7 +49,7 @@ def initialize( end def self.parse(rules, rate_limit, default_sample_rate) - rules = JSON.parse(rules).map do |rule| + parsed_rules = JSON.parse(rules).map do |rule| sample_rate = rule['sample_rate'] begin @@ -67,7 +67,7 @@ def self.parse(rules, rate_limit, default_sample_rate) SimpleRule.new(**kwargs) end - new(rules, rate_limit: rate_limit, default_sample_rate: default_sample_rate) + new(parsed_rules, rate_limit: rate_limit, default_sample_rate: default_sample_rate) rescue => e Datadog.logger.error do "Could not parse trace sampling rules '#{rules}': #{e.class.name} #{e.message} at #{Array(e.backtrace).first}" diff --git a/spec/datadog/tracing/sampling/rule_sampler_spec.rb b/spec/datadog/tracing/sampling/rule_sampler_spec.rb index b42977389ae..276363d34d7 100644 --- a/spec/datadog/tracing/sampling/rule_sampler_spec.rb +++ b/spec/datadog/tracing/sampling/rule_sampler_spec.rb @@ -77,7 +77,7 @@ end end - describe 'parse' do + describe '.parse' do subject(:parse) { described_class.parse(rules.to_json, rate_limit, default_sample_rate) } let(:rules) { [rule] } let(:rate_limit) { nil } @@ -104,7 +104,7 @@ end context 'and name' do - let(:rule) { super().merge(name: 'test-name') } + let(:rule) { { sample_rate: 0.1, name: 'test-name' } } it 'parses matching any service' do expect(actual_rule.matcher.name).to eq('test-name') @@ -114,7 +114,7 @@ end context 'and service' do - let(:rule) { super().merge(service: 'test-service') } + let(:rule) { { sample_rate: 0.1, service: 'test-service' } } it 'parses matching any name' do expect(actual_rule.matcher.name).to eq(Datadog::Tracing::Sampling::SimpleMatcher::MATCH_ALL) From efdeee4cf7678cf033cdfca168ffaabe364bca58 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Fri, 14 Jul 2023 14:43:11 -0700 Subject: [PATCH 20/22] Add compact backport --- lib/datadog/core/backport.rb | 10 ++++++++++ lib/datadog/tracing/sampling/rule_sampler.rb | 4 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/datadog/core/backport.rb b/lib/datadog/core/backport.rb index 88a36f3a1ff..1517e14095a 100644 --- a/lib/datadog/core/backport.rb +++ b/lib/datadog/core/backport.rb @@ -36,6 +36,16 @@ def self.dup(value) value.dup end end + + if ::Hash.method_defined?(:compact!) + def self.hash_compact!(hash) + hash.compact! + end + else + def self.hash_compact!(hash) + hash.reject! {|_key, value| value == nil} + end + end end end end diff --git a/lib/datadog/tracing/sampling/rule_sampler.rb b/lib/datadog/tracing/sampling/rule_sampler.rb index 698fec3b54d..61a405cd97c 100644 --- a/lib/datadog/tracing/sampling/rule_sampler.rb +++ b/lib/datadog/tracing/sampling/rule_sampler.rb @@ -62,7 +62,9 @@ def self.parse(rules, rate_limit, default_sample_rate) name: rule['name'], service: rule['service'], sample_rate: sample_rate, - }.compact + } + + Core::BackportFrom24.hash_compact!(kwargs) SimpleRule.new(**kwargs) end From 824c22d2887468fa293c35531f2da85fcf633eb6 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Fri, 14 Jul 2023 14:45:46 -0700 Subject: [PATCH 21/22] Lint --- lib/datadog/core/backport.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/datadog/core/backport.rb b/lib/datadog/core/backport.rb index 1517e14095a..0ebc7809f4c 100644 --- a/lib/datadog/core/backport.rb +++ b/lib/datadog/core/backport.rb @@ -43,7 +43,7 @@ def self.hash_compact!(hash) end else def self.hash_compact!(hash) - hash.reject! {|_key, value| value == nil} + hash.reject! { |_key, value| value.nil? } end end end From 9d95c961206a20a3dbc432c6b05f446d6e634b88 Mon Sep 17 00:00:00 2001 From: TonyCTHsu Date: Mon, 17 Jul 2023 13:29:20 +0200 Subject: [PATCH 22/22] Add spec for continuing tracing with empty TraceDigest (#2905) * Spec for Tracer#continue_trace! with empty digest * WIP * Revert "WIP" This reverts commit b0c8c1983cc77eb0c03cb47a9562bfe0a32fdbbd. --- spec/datadog/tracing/tracer_spec.rb | 43 +++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/spec/datadog/tracing/tracer_spec.rb b/spec/datadog/tracing/tracer_spec.rb index 67851832313..4c190a49d25 100644 --- a/spec/datadog/tracing/tracer_spec.rb +++ b/spec/datadog/tracing/tracer_spec.rb @@ -815,6 +815,49 @@ end end + context 'given empty TraceDigest' do + let(:digest) { Datadog::Tracing::TraceDigest.new } + + before { continue_trace! } + + it 'starts a new trace' do + tracer.trace('operation') do |span, trace| + expect(trace).to have_attributes( + origin: nil, + sampling_priority: 1 + ) + + expect(span).to have_attributes( + parent_id: 0, + span_id: a_kind_of(Integer), + trace_id: a_kind_of(Integer) + ) + end + + expect(tracer.active_trace).to be nil + end + + context 'and a block' do + it do + expect { |b| tracer.continue_trace!(digest, &b) } + .to yield_control + end + + it 'restores the original active trace afterwards' do + tracer.continue_trace!(digest) + original_trace = tracer.active_trace + expect(original_trace).to be_a_kind_of(Datadog::Tracing::TraceOperation) + + tracer.continue_trace!(digest) do + expect(tracer.active_trace).to be_a_kind_of(Datadog::Tracing::TraceOperation) + expect(tracer.active_trace).to_not be original_trace + end + + expect(tracer.active_trace).to be original_trace + end + end + end + context 'given a TraceDigest' do let(:digest) do Datadog::Tracing::TraceDigest.new(