Commit 625bdc5a authored by Andrew Newdigate's avatar Andrew Newdigate

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.
parent 35c3cb7c
---
title: Avoid overwriting default jaeger values with nil
merge_request: 24482
author:
type: fixed
...@@ -22,7 +22,7 @@ module Gitlab ...@@ -22,7 +22,7 @@ module Gitlab
service_name: service_name, service_name: service_name,
sampler: get_sampler(options[:sampler], options[:sampler_param]), sampler: get_sampler(options[:sampler], options[:sampler_param]),
reporter: get_reporter(service_name, options[:http_endpoint], options[:udp_endpoint]) 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 extra_params = options.except(:sampler, :sampler_param, :http_endpoint, :udp_endpoint, :strict_parsing, :debug) # rubocop: disable CodeReuse/ActiveRecord
if extra_params.present? if extra_params.present?
......
...@@ -6,36 +6,62 @@ describe Gitlab::Tracing::JaegerFactory do ...@@ -6,36 +6,62 @@ describe Gitlab::Tracing::JaegerFactory do
describe '.create_tracer' do describe '.create_tracer' do
let(:service_name) { 'rspec' } let(:service_name) { 'rspec' }
it 'processes default connections' do shared_examples_for 'a jaeger tracer' do
expect(described_class.create_tracer(service_name, {})).to respond_to(:active_span) 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 end
it 'handles debug options' do context 'handles debug options' do
expect(described_class.create_tracer(service_name, { debug: "1" })).to respond_to(:active_span) it_behaves_like 'a jaeger tracer' do
let(:tracer) { described_class.create_tracer(service_name, { debug: "1" }) }
end
end end
it 'handles const sampler' do context 'handles const sampler' do
expect(described_class.create_tracer(service_name, { sampler: "const", sampler_param: "1" })).to respond_to(:active_span) it_behaves_like 'a jaeger tracer' do
let(:tracer) { described_class.create_tracer(service_name, { sampler: "const", sampler_param: "1" }) }
end
end end
it 'handles probabilistic sampler' do context 'handles probabilistic sampler' do
expect(described_class.create_tracer(service_name, { sampler: "probabilistic", sampler_param: "0.5" })).to respond_to(:active_span) it_behaves_like 'a jaeger tracer' do
let(:tracer) { described_class.create_tracer(service_name, { sampler: "probabilistic", sampler_param: "0.5" }) }
end
end end
it 'handles http_endpoint configurations' do context 'handles http_endpoint configurations' do
expect(described_class.create_tracer(service_name, { http_endpoint: "http://localhost:1234" })).to respond_to(:active_span) it_behaves_like 'a jaeger tracer' do
let(:tracer) { described_class.create_tracer(service_name, { http_endpoint: "http://localhost:1234" }) }
end
end end
it 'handles udp_endpoint configurations' do context 'handles udp_endpoint configurations' do
expect(described_class.create_tracer(service_name, { udp_endpoint: "localhost:4321" })).to respond_to(:active_span) it_behaves_like 'a jaeger tracer' do
let(:tracer) { described_class.create_tracer(service_name, { udp_endpoint: "localhost:4321" }) }
end
end end
it 'ignores invalid parameters' do context 'ignores invalid parameters' do
expect(described_class.create_tracer(service_name, { invalid: "true" })).to respond_to(:active_span) it_behaves_like 'a jaeger tracer' do
let(:tracer) { described_class.create_tracer(service_name, { invalid: "true" }) }
end
end end
it 'accepts the debug parameter when strict_parser is set' do context '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) it_behaves_like 'a jaeger tracer' do
let(:tracer) { described_class.create_tracer(service_name, { debug: "1", strict_parsing: "1" }) }
end
end end
it 'rejects invalid parameters when strict_parser is set' do it 'rejects invalid parameters when strict_parser is set' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment