Skip to content

fix: Handle missing config in middleware when OTEL_SDK_DISABLED=true #2231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hannahramadan wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: Handle missing config in middleware when OTEL_SDK_DISABLED=true #2231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
779e2bb
fix: Handle unconfigured middleware when OTEL_SDK_DISABLED=true
hannahramadan Apr 14, 2026
8a360fd
Add empty config test
hannahramadan Apr 15, 2026
97f434c
Update base case, revert rack changes
hannahramadan Apr 16, 2026
a5ce8f6
grpc, gruf - return if not installed
hannahramadan Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions instrumentation/base/lib/opentelemetry/instrumentation/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,9 @@ def initialize(name, version, install_blk, present_blk,
@install_blk = install_blk
@present_blk = present_blk
@compatible_blk = compatible_blk
@config = {}
@installed = false
@options = options
@config = config_options({})
Copy link
Copy Markdown
Member Author

@hannahramadan hannahramadan Apr 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is a change to base, I would love some extra eyes here. I noted one downstream impact of checking for empty configs not really being able to exist anymore, since now they wont ever really be.

@installed = false
@tracer = OpenTelemetry::Trace::Tracer.new
end
# rubocop:enable Metrics/ParameterLists
Expand Down
9 changes: 9 additions & 0 deletions instrumentation/base/test/instrumentation/base_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,15 @@ def initialize(*args)
end
end

describe '#config' do
describe 'before install' do
it 'returns default values for defined options' do
instance = instrumentation_with_callbacks.instance
_(instance.config[:max_count]).must_equal(5)
end
end
end

describe '#enabled?' do
describe 'with env var' do
it 'is disabled when false' do
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/grpc/lib/opentelemetry/instrumentation/grpc/interceptors/client_tracer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def bidi_streamer(requests: nil, call: nil, method: nil, metadata: nil, &)
private

def call(type:, requests: nil, call: nil, method: nil, metadata: nil)
return yield if instrumentation_config.empty?
return yield unless Grpc::Instrumentation.instance.installed?
Copy link
Copy Markdown
Member Author

@hannahramadan hannahramadan Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check to see if instrumentation was installed, along with the checks in gruf/interceptors/client and gruf/interceptors/server aren't technically equivalent to the previous behavior of checking if the configs were empty.

Is installed? the right check here, or should we handle this differently? I'm unsure about the original intent of using an config empty vs installed to guard.

Copy link
Copy Markdown
Contributor

@arielvalentin arielvalentin Apr 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AS-AlStar would you mind sharing more information about your intent here?


method_parts = method.to_s.split('/')
service = method_parts[1]
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/gruf/lib/opentelemetry/instrumentation/gruf/interceptors/client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module Gruf
module Interceptors
class Client < ::Gruf::Interceptors::ClientInterceptor
def call(request_context:)
return yield if instrumentation_config.empty?
return yield unless Gruf::Instrumentation.instance.installed?

service = request_context.method.split('/')[1]
method = request_context.method_name
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/gruf/lib/opentelemetry/instrumentation/gruf/interceptors/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module Gruf
module Interceptors
class Server < ::Gruf::Interceptors::ServerInterceptor
def call
return yield if instrumentation_config.empty?
return yield unless Gruf::Instrumentation.instance.installed?

method = request.method_name

Expand Down
16 changes: 16 additions & 0 deletions ...on/rack/test/opentelemetry/instrumentation/rack/middlewares/dup/tracer_middleware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,4 +415,20 @@ class SimulatedError < StandardError
_(first_span.status.code).must_equal OpenTelemetry::Trace::Status::ERROR
end
end

describe 'when SDK is disabled' do
let(:disabled_rack_builder) { Rack::Builder.new }

before do
instrumentation.instance_variable_set(:@installed, false)
described_class.send(:clear_cached_config)
disabled_rack_builder.run app
disabled_rack_builder.use described_class
end

it 'handles requests without raising an error' do
response = Rack::MockRequest.new(disabled_rack_builder).get('/ping', env)
_(response.status).must_equal 200
end
end
end
16 changes: 16 additions & 0 deletions ...on/rack/test/opentelemetry/instrumentation/rack/middlewares/old/tracer_middleware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,4 +387,20 @@ class SimulatedError < StandardError
_(first_span.status.code).must_equal OpenTelemetry::Trace::Status::ERROR
end
end

describe 'when SDK is disabled' do
let(:disabled_rack_builder) { Rack::Builder.new }

before do
instrumentation.instance_variable_set(:@installed, false)
described_class.send(:clear_cached_config)
disabled_rack_builder.run app
disabled_rack_builder.use described_class
end

it 'handles requests without raising an error' do
response = Rack::MockRequest.new(disabled_rack_builder).get('/ping', env)
_(response.status).must_equal 200
end
end
end
16 changes: 16 additions & 0 deletions ...rack/test/opentelemetry/instrumentation/rack/middlewares/stable/tracer_middleware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,4 +386,20 @@ class SimulatedError < StandardError
_(first_span.status.code).must_equal OpenTelemetry::Trace::Status::ERROR
end
end

describe 'when SDK is disabled' do
let(:disabled_rack_builder) { Rack::Builder.new }

before do
instrumentation.instance_variable_set(:@installed, false)
described_class.send(:clear_cached_config)
disabled_rack_builder.run app
disabled_rack_builder.use described_class
end

it 'handles requests without raising an error' do
response = Rack::MockRequest.new(disabled_rack_builder).get('/ping', env)
_(response.status).must_equal 200
end
end
end
Loading