Commit c3f601e8 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'fix-method-instrumentation' into 'master'

Use Module#prepend for method instrumentation

cc @pcarranza 

See merge request !3754
parents 6d899f46 7b6785b3
Please view this file on the master branch, on stable branches it's out of date. Please view this file on the master branch, on stable branches it's out of date.
v 8.7.0 (unreleased) v 8.7.0 (unreleased)
- Method instrumentation now uses Module#prepend instead of aliasing methods
- The Projects::HousekeepingService class has extra instrumentation - The Projects::HousekeepingService class has extra instrumentation
- All service classes (those residing in app/services) are now instrumented - All service classes (those residing in app/services) are now instrumented
- Developers can now add custom tags to transactions - Developers can now add custom tags to transactions
......
...@@ -11,6 +11,8 @@ module Gitlab ...@@ -11,6 +11,8 @@ module Gitlab
module Instrumentation module Instrumentation
SERIES = 'method_calls' SERIES = 'method_calls'
PROXY_IVAR = :@__gitlab_instrumentation_proxy
def self.configure def self.configure
yield self yield self
end end
...@@ -91,6 +93,18 @@ module Gitlab ...@@ -91,6 +93,18 @@ module Gitlab
end end
end end
# Returns true if a module is instrumented.
#
# mod - The module to check
def self.instrumented?(mod)
mod.instance_variable_defined?(PROXY_IVAR)
end
# Returns the proxy module (if any) of `mod`.
def self.proxy_module(mod)
mod.instance_variable_get(PROXY_IVAR)
end
# Instruments a method. # Instruments a method.
# #
# type - The type (:class or :instance) of method to instrument. # type - The type (:class or :instance) of method to instrument.
...@@ -100,7 +114,6 @@ module Gitlab ...@@ -100,7 +114,6 @@ module Gitlab
return unless Metrics.enabled? return unless Metrics.enabled?
name = name.to_sym name = name.to_sym
alias_name = :"_original_#{name}"
target = type == :instance ? mod : mod.singleton_class target = type == :instance ? mod : mod.singleton_class
if type == :instance if type == :instance
...@@ -113,6 +126,12 @@ module Gitlab ...@@ -113,6 +126,12 @@ module Gitlab
method = mod.method(name) method = mod.method(name)
end end
unless instrumented?(target)
target.instance_variable_set(PROXY_IVAR, Module.new)
end
proxy_module = self.proxy_module(target)
# Some code out there (e.g. the "state_machine" Gem) checks the arity of # Some code out there (e.g. the "state_machine" Gem) checks the arity of
# a method to make sure it only passes arguments when the method expects # a method to make sure it only passes arguments when the method expects
# any. If we were to always overwrite a method to take an `*args` # any. If we were to always overwrite a method to take an `*args`
...@@ -125,17 +144,13 @@ module Gitlab ...@@ -125,17 +144,13 @@ module Gitlab
args_signature = '*args, &block' args_signature = '*args, &block'
end end
send_signature = "__send__(#{alias_name.inspect}, #{args_signature})" proxy_module.class_eval <<-EOF, __FILE__, __LINE__ + 1
target.class_eval <<-EOF, __FILE__, __LINE__ + 1
alias_method #{alias_name.inspect}, #{name.inspect}
def #{name}(#{args_signature}) def #{name}(#{args_signature})
trans = Gitlab::Metrics::Instrumentation.transaction trans = Gitlab::Metrics::Instrumentation.transaction
if trans if trans
start = Time.now start = Time.now
retval = #{send_signature} retval = super
duration = (Time.now - start) * 1000.0 duration = (Time.now - start) * 1000.0
if duration >= Gitlab::Metrics.method_call_threshold if duration >= Gitlab::Metrics.method_call_threshold
...@@ -148,10 +163,12 @@ module Gitlab ...@@ -148,10 +163,12 @@ module Gitlab
retval retval
else else
#{send_signature} super
end end
end end
EOF EOF
target.prepend(proxy_module)
end end
# Small layer of indirection to make it easier to stub out the current # Small layer of indirection to make it easier to stub out the current
......
...@@ -33,8 +33,16 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -33,8 +33,16 @@ describe Gitlab::Metrics::Instrumentation do
described_class.instrument_method(@dummy, :foo) described_class.instrument_method(@dummy, :foo)
end end
it 'renames the original method' do it 'instruments the Class' do
expect(@dummy).to respond_to(:_original_foo) target = @dummy.singleton_class
expect(described_class.instrumented?(target)).to eq(true)
end
it 'defines a proxy method' do
mod = described_class.proxy_module(@dummy.singleton_class)
expect(mod.method_defined?(:foo)).to eq(true)
end end
it 'calls the instrumented method with the correct arguments' do it 'calls the instrumented method with the correct arguments' do
...@@ -76,6 +84,14 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -76,6 +84,14 @@ describe Gitlab::Metrics::Instrumentation do
expect(dummy.method(:test).arity).to eq(0) expect(dummy.method(:test).arity).to eq(0)
end end
describe 'when a module is instrumented multiple times' do
it 'calls the instrumented method with the correct arguments' do
described_class.instrument_method(@dummy, :foo)
expect(@dummy.foo).to eq('foo')
end
end
end end
describe 'with metrics disabled' do describe 'with metrics disabled' do
...@@ -86,7 +102,9 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -86,7 +102,9 @@ describe Gitlab::Metrics::Instrumentation do
it 'does not instrument the method' do it 'does not instrument the method' do
described_class.instrument_method(@dummy, :foo) described_class.instrument_method(@dummy, :foo)
expect(@dummy).to_not respond_to(:_original_foo) target = @dummy.singleton_class
expect(described_class.instrumented?(target)).to eq(false)
end end
end end
end end
...@@ -100,8 +118,14 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -100,8 +118,14 @@ describe Gitlab::Metrics::Instrumentation do
instrument_instance_method(@dummy, :bar) instrument_instance_method(@dummy, :bar)
end end
it 'renames the original method' do it 'instruments instances of the Class' do
expect(@dummy.method_defined?(:_original_bar)).to eq(true) expect(described_class.instrumented?(@dummy)).to eq(true)
end
it 'defines a proxy method' do
mod = described_class.proxy_module(@dummy)
expect(mod.method_defined?(:bar)).to eq(true)
end end
it 'calls the instrumented method with the correct arguments' do it 'calls the instrumented method with the correct arguments' do
...@@ -144,7 +168,7 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -144,7 +168,7 @@ describe Gitlab::Metrics::Instrumentation do
described_class. described_class.
instrument_instance_method(@dummy, :bar) instrument_instance_method(@dummy, :bar)
expect(@dummy.method_defined?(:_original_bar)).to eq(false) expect(described_class.instrumented?(@dummy)).to eq(false)
end end
end end
end end
...@@ -167,18 +191,17 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -167,18 +191,17 @@ describe Gitlab::Metrics::Instrumentation do
it 'recursively instruments a class hierarchy' do it 'recursively instruments a class hierarchy' do
described_class.instrument_class_hierarchy(@dummy) described_class.instrument_class_hierarchy(@dummy)
expect(@child1).to respond_to(:_original_child1_foo) expect(described_class.instrumented?(@child1.singleton_class)).to eq(true)
expect(@child2).to respond_to(:_original_child2_foo) expect(described_class.instrumented?(@child2.singleton_class)).to eq(true)
expect(@child1.method_defined?(:_original_child1_bar)).to eq(true) expect(described_class.instrumented?(@child1)).to eq(true)
expect(@child2.method_defined?(:_original_child2_bar)).to eq(true) expect(described_class.instrumented?(@child2)).to eq(true)
end end
it 'does not instrument the root module' do it 'does not instrument the root module' do
described_class.instrument_class_hierarchy(@dummy) described_class.instrument_class_hierarchy(@dummy)
expect(@dummy).to_not respond_to(:_original_foo) expect(described_class.instrumented?(@dummy)).to eq(false)
expect(@dummy.method_defined?(:_original_bar)).to eq(false)
end end
end end
...@@ -190,7 +213,7 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -190,7 +213,7 @@ describe Gitlab::Metrics::Instrumentation do
it 'instruments all public class methods' do it 'instruments all public class methods' do
described_class.instrument_methods(@dummy) described_class.instrument_methods(@dummy)
expect(@dummy).to respond_to(:_original_foo) expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true)
end end
it 'only instruments methods directly defined in the module' do it 'only instruments methods directly defined in the module' do
...@@ -223,7 +246,7 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -223,7 +246,7 @@ describe Gitlab::Metrics::Instrumentation do
it 'instruments all public instance methods' do it 'instruments all public instance methods' do
described_class.instrument_instance_methods(@dummy) described_class.instrument_instance_methods(@dummy)
expect(@dummy.method_defined?(:_original_bar)).to eq(true) expect(described_class.instrumented?(@dummy)).to eq(true)
end end
it 'only instruments methods directly defined in the module' do it 'only instruments methods directly defined in the module' 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