From 0402b903e7abf08489f32a292c5b4d71cead5759 Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 19 Mar 2020 17:16:26 -0400 Subject: [PATCH 1/4] Fixed: Tracer default tags overriding span :tags option. --- lib/ddtrace/tracer.rb | 2 +- spec/ddtrace/tracer_spec.rb | 39 +++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/ddtrace/tracer.rb b/lib/ddtrace/tracer.rb index c98790c612d..7708ca1f0d9 100644 --- a/lib/ddtrace/tracer.rb +++ b/lib/ddtrace/tracer.rb @@ -210,8 +210,8 @@ def start_span(name, options = {}) # child span span.parent = parent # sets service, trace_id, parent_id, sampled end - tags.each { |k, v| span.set_tag(k, v) } unless tags.empty? @tags.each { |k, v| span.set_tag(k, v) } unless @tags.empty? + tags.each { |k, v| span.set_tag(k, v) } unless tags.empty? span.start_time = start_time # this could at some point be optional (start_active_span vs start_manual_span) diff --git a/spec/ddtrace/tracer_spec.rb b/spec/ddtrace/tracer_spec.rb index 34de71d1096..e2988a068e5 100644 --- a/spec/ddtrace/tracer_spec.rb +++ b/spec/ddtrace/tracer_spec.rb @@ -49,6 +49,45 @@ end end + describe '#start_span' do + subject(:start_span) { tracer.start_span(name, options) } + let(:span) { start_span } + let(:name) { 'span.name' } + let(:options) { {} } + + it { is_expected.to be_a_kind_of(Datadog::Span) } + + context 'when :tags are given' do + let(:options) { super().merge(tags: tags) } + let(:tags) { { tag_name => tag_value } } + let(:tag_name) { 'my-tag' } + let(:tag_value) { 'my-value' } + + it { expect(span.get_tag(tag_name)).to eq(tag_value) } + + context 'and default tags are set on the tracer' do + let(:default_tags) { { default_tag_name => default_tag_value } } + let(:default_tag_name) { 'default_tag' } + let(:default_tag_value) { 'default_value' } + + before { tracer.set_tags(default_tags) } + + it 'includes both :tags and default tags' do + expect(span.get_tag(default_tag_name)).to eq(default_tag_value) + expect(span.get_tag(tag_name)).to eq(tag_value) + end + + context 'which conflicts with :tags' do + let(:tag_name) { default_tag_name } + + it 'uses the tag from :tags' do + expect(span.get_tag(tag_name)).to eq(tag_value) + end + end + end + end + end + describe '#trace' do let(:name) { 'span.name' } let(:options) { {} } From 635187e4e68608551a1a7e4300cf33a518ea6054 Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 19 Mar 2020 18:06:11 -0400 Subject: [PATCH 2/4] Fixed: Tracer#set_tags storing tags as both Strings and Symbols. --- lib/ddtrace/tracer.rb | 3 ++- spec/ddtrace/configuration/settings_spec.rb | 4 +-- spec/ddtrace/tracer_spec.rb | 29 +++++++++++++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/lib/ddtrace/tracer.rb b/lib/ddtrace/tracer.rb index 7708ca1f0d9..0701c7160bf 100644 --- a/lib/ddtrace/tracer.rb +++ b/lib/ddtrace/tracer.rb @@ -158,7 +158,8 @@ def default_service # # tracer.set_tags('env' => 'prod', 'component' => 'core') def set_tags(tags) - @tags.update(tags) + string_tags = Hash[tags.collect { |k, v| [k.to_s, v] }] + @tags.update(string_tags) end # Guess context and parent from child_of entry. diff --git a/spec/ddtrace/configuration/settings_spec.rb b/spec/ddtrace/configuration/settings_spec.rb index 71586a2f9b4..fd3155ae53d 100644 --- a/spec/ddtrace/configuration/settings_spec.rb +++ b/spec/ddtrace/configuration/settings_spec.rb @@ -77,8 +77,8 @@ expect(Datadog::Logger.log).to eq(custom_log) expect(tracer.writer.transport.current_api.adapter.hostname).to eq('tracer.host.com') expect(tracer.writer.transport.current_api.adapter.port).to eq(1234) - expect(tracer.tags[:env]).to eq(:config_test) - expect(tracer.tags[:foo]).to eq(:bar) + expect(tracer.tags['env']).to eq(:config_test) + expect(tracer.tags['foo']).to eq(:bar) end end diff --git a/spec/ddtrace/tracer_spec.rb b/spec/ddtrace/tracer_spec.rb index e2988a068e5..ad1dfa83d18 100644 --- a/spec/ddtrace/tracer_spec.rb +++ b/spec/ddtrace/tracer_spec.rb @@ -24,6 +24,35 @@ it { is_expected.to be_a_kind_of(Hash) } + context 'when equivalent String and Symbols are added' do + shared_examples 'equivalent tags' do + it 'retains the tag only as a String' do + is_expected.to include('host') + is_expected.to_not include(:host) + end + + it 'retains only the last value' do + is_expected.to include('host' => 'b') + end + end + + context 'with #set_tags' do + it_behaves_like 'equivalent tags' do + before do + tracer.set_tags('host' => 'a') + tracer.set_tags(host: 'b') + end + end + + it_behaves_like 'equivalent tags' do + before do + tracer.set_tags(host: 'a') + tracer.set_tags('host' => 'b') + end + end + end + end + context "when #{Datadog::Ext::Environment::ENV_ENVIRONMENT}" do context 'is not defined' do around do |example| From cc426ae19b07e97dc9ad6b5f05da3ca40f44a7c1 Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 19 Mar 2020 18:10:20 -0400 Subject: [PATCH 3/4] Added: Environment::env and Environment::tags. --- lib/ddtrace/environment.rb | 21 +++++++ lib/ddtrace/ext/environment.rb | 1 + spec/ddtrace/environment_spec.rb | 100 +++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 spec/ddtrace/environment_spec.rb diff --git a/lib/ddtrace/environment.rb b/lib/ddtrace/environment.rb index 29ebe22c962..c8ef7927047 100644 --- a/lib/ddtrace/environment.rb +++ b/lib/ddtrace/environment.rb @@ -1,6 +1,27 @@ +require 'ddtrace/ext/environment' + module Datadog # Namespace for handling application environment module Environment + # TODO: Extract to Datadog::Configuration::Settings + def self.env + ENV[Ext::Environment::ENV_ENVIRONMENT] + end + + # TODO: Extract to Datadog::Configuration::Settings + def self.tags + tags = {} + + env_to_list(Ext::Environment::ENV_TAGS).each do |tag| + pair = tag.split(':') + tags[pair.first] = pair.last if pair.length == 2 + end + + tags['env'] = env unless env.nil? + + tags + end + # Defines helper methods for environment module Helpers def env_to_bool(var, default = nil) diff --git a/lib/ddtrace/ext/environment.rb b/lib/ddtrace/ext/environment.rb index 6a3f2d7c5eb..52a06542577 100644 --- a/lib/ddtrace/ext/environment.rb +++ b/lib/ddtrace/ext/environment.rb @@ -2,6 +2,7 @@ module Datadog module Ext module Environment ENV_ENVIRONMENT = 'DD_ENV'.freeze + ENV_TAGS = 'DD_TAGS'.freeze end end end diff --git a/spec/ddtrace/environment_spec.rb b/spec/ddtrace/environment_spec.rb new file mode 100644 index 00000000000..3418d23c295 --- /dev/null +++ b/spec/ddtrace/environment_spec.rb @@ -0,0 +1,100 @@ +require 'spec_helper' + +require 'ddtrace' +require 'ddtrace/environment' + +RSpec.describe Datadog::Environment do + describe '::env' do + subject(:env) { described_class.env } + context "when #{Datadog::Ext::Environment::ENV_ENVIRONMENT}" do + around do |example| + ClimateControl.modify(Datadog::Ext::Environment::ENV_ENVIRONMENT => environment) do + example.run + end + end + + context 'is not defined' do + let(:environment) { nil } + it { is_expected.to be nil } + end + + context 'is defined' do + let(:environment) { 'env-value' } + it { is_expected.to eq(environment) } + end + end + end + + describe '::tags' do + subject(:tags) { described_class.tags } + + context "when #{Datadog::Ext::Environment::ENV_TAGS}" do + around do |example| + ClimateControl.modify(Datadog::Ext::Environment::ENV_TAGS => env_tags) do + example.run + end + end + + context 'is not defined' do + let(:env_tags) { nil } + it { is_expected.to eq({}) } + end + + context 'is defined' do + let(:env_tags) { 'a:1,b:2' } + + it { is_expected.to include('a' => '1', 'b' => '2') } + + context 'with an invalid tag' do + context do + let(:env_tags) { '' } + it { is_expected.to eq({}) } + end + + context do + let(:env_tags) { 'a' } + it { is_expected.to eq({}) } + end + + context do + let(:env_tags) { ':' } + it { is_expected.to eq({}) } + end + + context do + let(:env_tags) { ',' } + it { is_expected.to eq({}) } + end + + context do + let(:env_tags) { 'a:' } + it { is_expected.to eq({}) } + end + end + + context 'and when ::env' do + context 'is set' do + before { allow(described_class).to receive(:env).and_return(nil) } + it { is_expected.to_not include('env') } + end + + context 'is not set' do + let(:env_value) { 'env-value' } + before { allow(described_class).to receive(:env).and_return(env_value) } + it { is_expected.to include('env' => env_value) } + end + end + end + + context 'conflicts with ::env' do + let(:env_tags) { "env:#{tag_env_value}" } + let(:tag_env_value) { 'tag-env-value' } + let(:env_value) { 'env-value' } + + before { allow(described_class).to receive(:env).and_return(env_value) } + + it { is_expected.to include('env' => env_value) } + end + end + end +end From a22c2113961cff77f06ff4fffb7626a38a1ba443 Mon Sep 17 00:00:00 2001 From: David Elner Date: Thu, 19 Mar 2020 18:28:50 -0400 Subject: [PATCH 4/4] Changed: Use from env var to Datadog::Environment. --- lib/ddtrace/correlation.rb | 4 ++-- lib/ddtrace/metrics.rb | 4 ++-- lib/ddtrace/tracer.rb | 7 ++----- spec/ddtrace/correlation_spec.rb | 18 ++++-------------- spec/ddtrace/metrics_spec.rb | 18 ++++-------------- spec/ddtrace/tracer_spec.rb | 31 ++++++------------------------- 6 files changed, 20 insertions(+), 62 deletions(-) diff --git a/lib/ddtrace/correlation.rb b/lib/ddtrace/correlation.rb index 86ec24eafcc..01e0ff5ab0c 100644 --- a/lib/ddtrace/correlation.rb +++ b/lib/ddtrace/correlation.rb @@ -1,4 +1,4 @@ -require 'ddtrace/ext/environment' +require 'ddtrace/environment' module Datadog # Contains behavior for managing correlations with tracing @@ -10,7 +10,7 @@ def initialize(*args) super self.trace_id = trace_id || 0 self.span_id = span_id || 0 - self.env = env || ENV[Datadog::Ext::Environment::ENV_ENVIRONMENT] + self.env = env || Datadog::Environment.env end def to_s diff --git a/lib/ddtrace/metrics.rb b/lib/ddtrace/metrics.rb index ac3d43e0270..cea65a1e199 100644 --- a/lib/ddtrace/metrics.rb +++ b/lib/ddtrace/metrics.rb @@ -1,8 +1,8 @@ require 'ddtrace/ext/metrics' -require 'ddtrace/ext/environment' require 'set' require 'logger' +require 'ddtrace/environment' require 'ddtrace/utils/time' require 'ddtrace/runtime/identity' @@ -152,7 +152,7 @@ def default_metric_options # and defaults are unfrozen for mutation in Statsd. DEFAULT.dup.tap do |options| options[:tags] = options[:tags].dup - options[:tags] << "env:#{ENV[Ext::Environment::ENV_ENVIRONMENT]}" if ENV.key?(Ext::Environment::ENV_ENVIRONMENT) + options[:tags] << "env:#{Datadog::Environment.env}" unless Datadog::Environment.env.nil? end end end diff --git a/lib/ddtrace/tracer.rb b/lib/ddtrace/tracer.rb index 0701c7160bf..ae54373c173 100644 --- a/lib/ddtrace/tracer.rb +++ b/lib/ddtrace/tracer.rb @@ -3,8 +3,7 @@ require 'logger' require 'pathname' -require 'ddtrace/ext/environment' - +require 'ddtrace/environment' require 'ddtrace/span' require 'ddtrace/context' require 'ddtrace/logger' @@ -85,9 +84,7 @@ def initialize(options = {}) end @mutex = Mutex.new - @tags = {}.tap do |tags| - tags[:env] = ENV[Ext::Environment::ENV_ENVIRONMENT] if ENV.key?(Ext::Environment::ENV_ENVIRONMENT) - end + @tags = Datadog::Environment.tags # Enable priority sampling by default activate_priority_sampling!(@sampler) diff --git a/spec/ddtrace/correlation_spec.rb b/spec/ddtrace/correlation_spec.rb index a8192c8b66b..a5ee93846d9 100644 --- a/spec/ddtrace/correlation_spec.rb +++ b/spec/ddtrace/correlation_spec.rb @@ -37,27 +37,17 @@ expect(correlation_ids.to_s).to eq("dd.trace_id=#{trace_id} dd.span_id=#{span_id}") end - context "when #{Datadog::Ext::Environment::ENV_ENVIRONMENT}" do - context 'is not defined' do - around do |example| - ClimateControl.modify(Datadog::Ext::Environment::ENV_ENVIRONMENT => nil) do - example.run - end - end + context 'when Datadog::Environment.env' do + before { allow(Datadog::Environment).to receive(:env).and_return(environment) } + context 'is not defined' do + let(:environment) { nil } it { expect(correlation_ids.env).to be nil } it { expect(correlation_ids.to_s).to eq("dd.trace_id=#{trace_id} dd.span_id=#{span_id}") } end context 'is defined' do let(:environment) { 'my-env' } - - around do |example| - ClimateControl.modify(Datadog::Ext::Environment::ENV_ENVIRONMENT => environment) do - example.run - end - end - it { expect(correlation_ids.env).to eq environment } it { expect(correlation_ids.to_s).to eq("dd.trace_id=#{trace_id} dd.span_id=#{span_id} dd.env=#{environment}") } end diff --git a/spec/ddtrace/metrics_spec.rb b/spec/ddtrace/metrics_spec.rb index f29d085b9cc..a7dc9c223ff 100644 --- a/spec/ddtrace/metrics_spec.rb +++ b/spec/ddtrace/metrics_spec.rb @@ -638,26 +638,16 @@ ) end - context "when #{Datadog::Ext::Environment::ENV_ENVIRONMENT}" do - context 'is not defined' do - around do |example| - ClimateControl.modify(Datadog::Ext::Environment::ENV_ENVIRONMENT => nil) do - example.run - end - end + context 'when Datadog::Environment.env' do + before { allow(Datadog::Environment).to receive(:env).and_return(environment) } + context 'is not defined' do + let(:environment) { nil } it { is_expected.to_not include(/env:/) } end context 'is defined' do let(:environment) { 'my-env' } - - around do |example| - ClimateControl.modify(Datadog::Ext::Environment::ENV_ENVIRONMENT => environment) do - example.run - end - end - it { is_expected.to include("env:#{environment}") } end end diff --git a/spec/ddtrace/tracer_spec.rb b/spec/ddtrace/tracer_spec.rb index ad1dfa83d18..8ace05b0128 100644 --- a/spec/ddtrace/tracer_spec.rb +++ b/spec/ddtrace/tracer_spec.rb @@ -21,8 +21,13 @@ describe '#tags' do subject(:tags) { tracer.tags } + let(:env_tags) { {} } - it { is_expected.to be_a_kind_of(Hash) } + before { allow(Datadog::Environment).to receive(:tags).and_return(env_tags) } + + context 'by default' do + it { is_expected.to be env_tags } + end context 'when equivalent String and Symbols are added' do shared_examples 'equivalent tags' do @@ -52,30 +57,6 @@ end end end - - context "when #{Datadog::Ext::Environment::ENV_ENVIRONMENT}" do - context 'is not defined' do - around do |example| - ClimateControl.modify(Datadog::Ext::Environment::ENV_ENVIRONMENT => nil) do - example.run - end - end - - it { is_expected.to_not include(:env) } - end - - context 'is defined' do - let(:environment) { 'my-env' } - - around do |example| - ClimateControl.modify(Datadog::Ext::Environment::ENV_ENVIRONMENT => environment) do - example.run - end - end - - it { is_expected.to include(env: environment) } - end - end end describe '#start_span' do