Avoid overwriting default jaeger values with nil

During the review process for adding opentracing factories, a bug was
introduced which caused Jaeger to initialize an invalid tracer. The
bug was due to use sending nil through as a kwarg when the Jaeger
initializer used a non-nil default value.

This is fairly insidious as, the tracer looks like a tracer, but, when
methods are invoked, it throws `NoMethodError` errors. To ensure that
this issue does not happen in future, the tests have been changed to
ensure that the tracer works as expected. This could avoid problems
in future when upgrading to newer versions of Jaeger.

3 files changed, 48 insertions, 17 deletions

diff --git a/changelogs/unreleased/an-dtracing-test-for-invalid-tracers.yml b/changelogs/unreleased/an-dtracing-test-for-invalid-tracers.yml
new file mode 100644
index 00000000000..5365260cbae
--- /dev/null
+++ b/changelogs/unreleased/an-dtracing-test-for-invalid-tracers.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid overwriting default jaeger values with nil
+merge_request: 24482
+author:
+type: fixed
diff --git a/lib/gitlab/tracing/jaeger_factory.rb b/lib/gitlab/tracing/jaeger_factory.rb
index 0726f6b67f4..2682007302a 100644
--- a/lib/gitlab/tracing/jaeger_factory.rb
+++ b/lib/gitlab/tracing/jaeger_factory.rb
@@ -22,7 +22,7 @@ module Gitlab
           service_name: service_name,
           sampler: get_sampler(options[:sampler], options[:sampler_param]),
           reporter: get_reporter(service_name, options[:http_endpoint], options[:udp_endpoint])
-        }
+        }.compact
 
         extra_params = options.except(:sampler, :sampler_param, :http_endpoint, :udp_endpoint, :strict_parsing, :debug) # rubocop: disable CodeReuse/ActiveRecord
         if extra_params.present?
diff --git a/spec/lib/gitlab/tracing/jaeger_factory_spec.rb b/spec/lib/gitlab/tracing/jaeger_factory_spec.rb
index 3bffeb28830..3d6a007cfd9 100644
--- a/spec/lib/gitlab/tracing/jaeger_factory_spec.rb
+++ b/spec/lib/gitlab/tracing/jaeger_factory_spec.rb
@@ -6,36 +6,62 @@ describe Gitlab::Tracing::JaegerFactory do
   describe '.create_tracer' do
     let(:service_name) { 'rspec' }
 
-    it 'processes default connections' do
-      expect(described_class.create_tracer(service_name, {})).to respond_to(:active_span)
+    shared_examples_for 'a jaeger tracer' do
+      it 'responds to active_span methods' do
+        expect(tracer).to respond_to(:active_span)
+      end
+
+      it 'yields control' do
+        expect { |b| tracer.start_active_span('operation_name', &b) }.to yield_control
+      end
+    end
+
+    context 'processes default connections' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, {}) }
+      end
     end
 
-    it 'handles debug options' do
-      expect(described_class.create_tracer(service_name, { debug: "1" })).to respond_to(:active_span)
+    context 'handles debug options' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, { debug: "1" }) }
+      end
     end
 
-    it 'handles const sampler' do
-      expect(described_class.create_tracer(service_name, { sampler: "const", sampler_param: "1" })).to respond_to(:active_span)
+    context 'handles const sampler' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, { sampler: "const", sampler_param: "1" }) }
+      end
     end
 
-    it 'handles probabilistic sampler' do
-      expect(described_class.create_tracer(service_name, { sampler: "probabilistic", sampler_param: "0.5" })).to respond_to(:active_span)
+    context 'handles probabilistic sampler' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, { sampler: "probabilistic", sampler_param: "0.5" }) }
+      end
     end
 
-    it 'handles http_endpoint configurations' do
-      expect(described_class.create_tracer(service_name, { http_endpoint: "http://localhost:1234" })).to respond_to(:active_span)
+    context 'handles http_endpoint configurations' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, { http_endpoint: "http://localhost:1234" }) }
+      end
     end
 
-    it 'handles udp_endpoint configurations' do
-      expect(described_class.create_tracer(service_name, { udp_endpoint: "localhost:4321" })).to respond_to(:active_span)
+    context 'handles udp_endpoint configurations' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, { udp_endpoint: "localhost:4321" }) }
+      end
     end
 
-    it 'ignores invalid parameters' do
-      expect(described_class.create_tracer(service_name, { invalid: "true" })).to respond_to(:active_span)
+    context 'ignores invalid parameters' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, { invalid: "true" }) }
+      end
     end
 
-    it 'accepts the debug parameter when strict_parser is set' do
-      expect(described_class.create_tracer(service_name, { debug: "1", strict_parsing: "1" })).to respond_to(:active_span)
+    context 'accepts the debug parameter when strict_parser is set' do
+      it_behaves_like 'a jaeger tracer' do
+        let(:tracer) { described_class.create_tracer(service_name, { debug: "1", strict_parsing: "1" }) }
+      end
     end
 
     it 'rejects invalid parameters when strict_parser is set' do