Skip to content

Commit add4289

Browse files
committed
[PROF-3514] Skip CPU time instrumentation if logging gem is detected
Instead, customers will get the following log message: ``` INFO -- ddtrace: [ddtrace] (/app/lib/ddtrace/profiling/tasks/setup.rb :44:in `activate_cpu_extensions') CPU time profiling skipped because native CPU time is not supported: 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 TwP/logging#230 for details. Profiles containing Wall time will still be reported. ``` 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 add4289

File tree

2 files changed

+61
-2
lines changed

2 files changed

+61
-2
lines changed

lib/ddtrace/profiling/ext/cpu.rb

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,24 @@ def self.unsupported_reason
4343
elsif Gem::Specification.find_all_by_name('rollbar', ROLLBAR_INCOMPATIBLE_VERSIONS).any?
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. " \
46-
'See https://github.com/rollbar/rollbar-gem/pull/1018 for details.'
46+
'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+
"The `logging` gem is installed and its thread inherit context feature is enabled. " \
49+
"Please add LOGGING_INHERIT_CONTEXT=false to your application's environment variables to disable the " \
50+
'conflicting `logging` gem 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 '`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 '`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)