Skip to content

Commit b7f7b01

Browse files
committed
[PROF-3514] Skip CPU time instrumentation if logging gem is detected
This is a repeat of #1362 -- the logging gem also monkey patches `Thread` initialization using `alias_method`, which is incompatible with our instrumentation that uses `preload`. For details, see TwP/logging#230 where we submitted a fix to the upstream gem. Right now the only way to avoid this issue and still enable proflier is to either remove the logging gem or to disable the feature using the `LOGGING_INHERIT_CONTEXT` environment variable. If/when the gem creators accept our proposed change and release it, we'll need to revise this to take that into consideration, as we do for rollbar. NOTE: I'm not entirely happy with the complexity that is accumulating for the tests, but I've decided to not refactor it yet. If we need to add more gems, we should definitely refactor the tests. My hope is that soon we can get rid of our `Thread` monkey patches, and thus get rid of all of this instead.
1 parent e702645 commit b7f7b01

File tree

2 files changed

+60
-1
lines changed

2 files changed

+60
-1
lines changed

lib/ddtrace/profiling/ext/cpu.rb

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,23 @@ def self.unsupported_reason
4444
'You have an incompatible rollbar gem version installed; ensure that you have rollbar >= 3.1.2 by ' \
4545
"adding `gem 'rollbar', '>= 3.1.2'` to your Gemfile or gems.rb file. " \
4646
'See https://github.com/rollbar/rollbar-gem/pull/1018 for details.'
47+
elsif Gem::Specification.find_all_by_name('logging').any? && logging_inherit_context_enabled?
48+
"Feature is incompatible with the `logging` gem's thread inherit context feature. " \
49+
"Please add LOGGING_INHERIT_CONTEXT=false to your application's environment variables to disable the " \
50+
'conflicting feature. ' \
51+
'See https://github.com/TwP/logging/pull/230 for details.'
4752
end
4853
end
54+
55+
private_class_method def self.logging_inherit_context_enabled?
56+
# The logging gem provides a mechanism to disable the conflicting behavior, see
57+
# https://github.com/TwP/logging/blob/ae9872d093833b2a5a34cbe1faa4e895a81f6845/lib/logging/diagnostic_context.rb#L418
58+
# Here we check if the behavior is enabled
59+
inherit_context_configuration = ENV['LOGGING_INHERIT_CONTEXT']
60+
61+
inherit_context_configuration.nil? ||
62+
(inherit_context_configuration && !%w[false no 0].include?(inherit_context_configuration.downcase))
63+
end
4964
end
5065
end
5166
end

spec/ddtrace/profiling/ext/cpu_spec.rb

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,51 @@
7777
.and_return([])
7878
end
7979

80-
it { is_expected.to be nil }
80+
context 'when logging gem is installed' do
81+
before do
82+
expect(Gem::Specification)
83+
.to receive(:find_all_by_name)
84+
.with('logging')
85+
.and_return([instance_double(Gem::Specification)])
86+
end
87+
88+
around do |example|
89+
ClimateControl.modify('LOGGING_INHERIT_CONTEXT' => logging_inherit_context) do
90+
example.run
91+
end
92+
end
93+
94+
context 'when no LOGGING_INHERIT_CONTEXT is defined' do
95+
let(:logging_inherit_context) { nil }
96+
97+
it { is_expected.to include 'incompatible with the `logging` gem' }
98+
end
99+
100+
context 'when LOGGING_INHERIT_CONTEXT is set to any value other than false/no/0 (case-insensitive)' do
101+
let(:logging_inherit_context) { 'YesPlease' }
102+
103+
it { is_expected.to include 'incompatible with the `logging` gem' }
104+
end
105+
106+
context 'when LOGGING_INHERIT_CONTEXT is set to false/no/0 (case-insensitive)' do
107+
%w[false no FaLsE nO 0].each do |disabled_setting|
108+
let(:logging_inherit_context) { disabled_setting }
109+
110+
it { is_expected.to be nil }
111+
end
112+
end
113+
end
114+
115+
context 'when logging gem is not installed' do
116+
before do
117+
expect(Gem::Specification)
118+
.to receive(:find_all_by_name)
119+
.with('logging')
120+
.and_return([])
121+
end
122+
123+
it { is_expected.to be nil }
124+
end
81125
end
82126
end
83127
end

0 commit comments

Comments
 (0)