From 95fefe234ca0376eec8b66fe85336c0719e3be5f Mon Sep 17 00:00:00 2001 From: David Elner Date: Mon, 11 May 2020 11:06:30 -0400 Subject: [PATCH] Merge pull request #1034 from DataDog/fix/configure_eager_loading_lazy_vars Fix #configure forcing all options to eager load --- lib/ddtrace/configuration/base.rb | 2 +- lib/ddtrace/configuration/options.rb | 2 +- lib/ddtrace/contrib/configuration/settings.rb | 2 +- .../contrib/rails/configuration/settings.rb | 13 +++++ .../contrib/configuration/settings_spec.rb | 58 ++++++++++++------- spec/ddtrace/contrib/rails/analytics_spec.rb | 2 + 6 files changed, 55 insertions(+), 24 deletions(-) diff --git a/lib/ddtrace/configuration/base.rb b/lib/ddtrace/configuration/base.rb index ed19f693dca..59a635255ab 100644 --- a/lib/ddtrace/configuration/base.rb +++ b/lib/ddtrace/configuration/base.rb @@ -45,7 +45,7 @@ def new_settings_class(&block) # Instance methods for configuration module InstanceMethods def initialize(options = {}) - configure(options) + configure(options) unless options.empty? end def configure(opts = {}) diff --git a/lib/ddtrace/configuration/options.rb b/lib/ddtrace/configuration/options.rb index 05b6009e141..7234ad25d6e 100644 --- a/lib/ddtrace/configuration/options.rb +++ b/lib/ddtrace/configuration/options.rb @@ -80,7 +80,7 @@ def option_defined?(name) end def options_hash - options.each_with_object({}) do |(key, _), hash| + self.class.options.merge(options).each_with_object({}) do |(key, _), hash| hash[key] = get_option(key) end end diff --git a/lib/ddtrace/contrib/configuration/settings.rb b/lib/ddtrace/contrib/configuration/settings.rb index 78a775f2b22..456679b1e3f 100644 --- a/lib/ddtrace/contrib/configuration/settings.rb +++ b/lib/ddtrace/contrib/configuration/settings.rb @@ -16,7 +16,7 @@ class Settings def configure(options = {}) self.class.options.dependency_order.each do |name| - self[name] = options.fetch(name, self[name]) + self[name] = options[name] if options.key?(name) end yield(self) if block_given? diff --git a/lib/ddtrace/contrib/rails/configuration/settings.rb b/lib/ddtrace/contrib/rails/configuration/settings.rb index 6caff476385..4847aa5038f 100644 --- a/lib/ddtrace/contrib/rails/configuration/settings.rb +++ b/lib/ddtrace/contrib/rails/configuration/settings.rb @@ -6,6 +6,19 @@ module Rails module Configuration # Custom settings for the Rails integration class Settings < Contrib::Configuration::Settings + def initialize(options = {}) + super(options) + + # NOTE: Eager load these + # Rails integration is responsible for orchestrating other integrations. + # When using environment variables, settings will not be automatically + # filled because nothing explicitly calls them. They must though, so + # integrations like ActionPack can receive the value as it should. + # Trigger these manually to force an eager load and propagate them. + analytics_enabled + analytics_sample_rate + end + option :analytics_enabled do |o| o.default { env_to_bool(Ext::ENV_ANALYTICS_ENABLED, nil) } o.lazy diff --git a/spec/ddtrace/contrib/configuration/settings_spec.rb b/spec/ddtrace/contrib/configuration/settings_spec.rb index a3264334881..674ede0b99d 100644 --- a/spec/ddtrace/contrib/configuration/settings_spec.rb +++ b/spec/ddtrace/contrib/configuration/settings_spec.rb @@ -7,31 +7,47 @@ it { is_expected.to be_a_kind_of(Datadog::Configuration::Base) } - describe '#options' do - subject(:options) { settings.options } + describe '#service_name' do + subject(:service_name) { settings.service_name } + it { expect(settings.service_name).to be nil } + it { expect(settings[:service_name]).to be nil } + end - describe ':service_name' do - subject(:option) { options[:service_name] } - it { expect(options).to include(:service_name) } - it { expect(option.get).to be nil } - end + describe '#tracer' do + subject(:tracer) { settings.tracer } + it { expect(settings.tracer).to be Datadog.tracer } + it { expect(settings[:tracer]).to be Datadog.tracer } + end - describe ':tracer' do - subject(:option) { options[:tracer] } - it { expect(options).to include(:tracer) } - it { expect(option.get).to be Datadog.tracer } - end + describe '#analytics_enabled' do + subject(:analytics_enabled) { settings.analytics_enabled } + it { expect(settings.analytics_enabled).to be false } + it { expect(settings[:analytics_enabled]).to be false } + end - describe ':analytics_enabled' do - subject(:option) { options[:analytics_enabled] } - it { expect(options).to include(:analytics_enabled) } - it { expect(option.get).to be false } - end + describe '#analytics_sample_rate' do + subject(:analytics_sample_rate) { settings.analytics_sample_rate } + it { expect(settings.analytics_sample_rate).to eq 1.0 } + it { expect(settings[:analytics_sample_rate]).to eq 1.0 } + end + + describe '#configure' do + subject(:configure) { settings.configure(options) } + + context 'given an option' do + let(:options) { { service_name: service_name } } + let(:service_name) { double('service_name') } + + before { allow(settings).to receive(:set_option).and_call_original } + + it 'doesn\'t initialize other options' do + expect { configure } + .to change { settings.service_name } + .from(nil) + .to(service_name) - describe ':analytics_sample_rate' do - subject(:option) { options[:analytics_sample_rate] } - it { expect(options).to include(:analytics_sample_rate) } - it { expect(option.get).to eq 1.0 } + expect(settings).to_not have_received(:set_option).with(:tracer, any_args) + end end end end diff --git a/spec/ddtrace/contrib/rails/analytics_spec.rb b/spec/ddtrace/contrib/rails/analytics_spec.rb index 50dd8703830..934bd183c6f 100644 --- a/spec/ddtrace/contrib/rails/analytics_spec.rb +++ b/spec/ddtrace/contrib/rails/analytics_spec.rb @@ -19,8 +19,10 @@ around do |example| # Reset before and after each example; don't allow global state to linger. Datadog.registry[:rails].reset_configuration! + Datadog.registry[:action_pack].reset_configuration! example.run Datadog.registry[:rails].reset_configuration! + Datadog.registry[:action_pack].reset_configuration! end let(:span) { spans.first }