Skip to content
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.

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

[PROF-3514] Skip CPU time instrumentation if logging gem is detected #1557

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 16 additions & 1 deletion lib/ddtrace/profiling/ext/cpu.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,24 @@ def self.unsupported_reason
elsif Gem::Specification.find_all_by_name('rollbar', ROLLBAR_INCOMPATIBLE_VERSIONS).any?
'You have an incompatible rollbar gem version installed; ensure that you have rollbar >= 3.1.2 by ' \
"adding `gem 'rollbar', '>= 3.1.2'` to your Gemfile or gems.rb file. " \
'See https://github.com/rollbar/rollbar-gem/pull/1018 for details.'
'See https://github.com/rollbar/rollbar-gem/pull/1018 for details'
elsif Gem::Specification.find_all_by_name('logging').any? && logging_inherit_context_enabled?
'The `logging` gem is installed and its thread inherit context feature is enabled. ' \
"Please add LOGGING_INHERIT_CONTEXT=false to your application's environment variables to disable the " \
'conflicting `logging` gem feature. ' \
'See https://github.com/TwP/logging/pull/230 for details'
end
end

private_class_method def self.logging_inherit_context_enabled?
# The logging gem provides a mechanism to disable the conflicting behavior, see
# https://github.com/TwP/logging/blob/ae9872d093833b2a5a34cbe1faa4e895a81f6845/lib/logging/diagnostic_context.rb#L418
# Here we check if the behavior is enabled
inherit_context_configuration = ENV['LOGGING_INHERIT_CONTEXT']

inherit_context_configuration.nil? ||
(inherit_context_configuration && !%w[false no 0].include?(inherit_context_configuration.downcase))
end
end
end
end
Expand Down
46 changes: 45 additions & 1 deletion spec/ddtrace/profiling/ext/cpu_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,51 @@
.and_return([])
end

it { is_expected.to be nil }
context 'when logging gem is installed' do
before do
expect(Gem::Specification)
.to receive(:find_all_by_name)
.with('logging')
.and_return([instance_double(Gem::Specification)])
end

around do |example|
ClimateControl.modify('LOGGING_INHERIT_CONTEXT' => logging_inherit_context) do
example.run
end
end

context 'when no LOGGING_INHERIT_CONTEXT is defined' do
let(:logging_inherit_context) { nil }

it { is_expected.to include '`logging` gem' }
end

context 'when LOGGING_INHERIT_CONTEXT is set to any value other than false/no/0 (case-insensitive)' do
let(:logging_inherit_context) { 'YesPlease' }

it { is_expected.to include '`logging` gem' }
end

context 'when LOGGING_INHERIT_CONTEXT is set to false/no/0 (case-insensitive)' do
%w[false no FaLsE nO 0].each do |disabled_setting|
let(:logging_inherit_context) { disabled_setting }

it { is_expected.to be nil }
end
end
end

context 'when logging gem is not installed' do
before do
expect(Gem::Specification)
.to receive(:find_all_by_name)
.with('logging')
.and_return([])
end

it { is_expected.to be nil }
end
end
end
end
Expand Down