Commit dadc5313 authored by Paco Guzman's avatar Paco Guzman

Instrument private/protected methods

By default instrumentation will instrument public,
protected and private methods, because usually
heavy work is done on private method or at least
that’s what facts is showing
parent fdcafe72
...@@ -78,6 +78,7 @@ v 8.9.0 (unreleased) ...@@ -78,6 +78,7 @@ v 8.9.0 (unreleased)
- Remove deprecated issues_tracker and issues_tracker_id from project model - Remove deprecated issues_tracker and issues_tracker_id from project model
- Allow users to create confidential issues in private projects - Allow users to create confidential issues in private projects
- Measure CPU time for instrumented methods - Measure CPU time for instrumented methods
- Instrument private methods and private instance methods by default instead just public methods
v 8.8.5 (unreleased) v 8.8.5 (unreleased)
- Ensure branch cleanup regardless of whether the GitHub import process succeeds - Ensure branch cleanup regardless of whether the GitHub import process succeeds
......
...@@ -128,11 +128,6 @@ if Gitlab::Metrics.enabled? ...@@ -128,11 +128,6 @@ if Gitlab::Metrics.enabled?
config.instrument_instance_methods(API::Helpers) config.instrument_instance_methods(API::Helpers)
config.instrument_instance_methods(RepositoryCheck::SingleRepositoryWorker) config.instrument_instance_methods(RepositoryCheck::SingleRepositoryWorker)
# Iterate over each non-super private instance method to keep up to date if
# internals change
RepositoryCheck::SingleRepositoryWorker.private_instance_methods(false).each do |method|
config.instrument_instance_method(RepositoryCheck::SingleRepositoryWorker, method)
end
end end
GC::Profiler.enable GC::Profiler.enable
......
...@@ -15,8 +15,8 @@ instrument code: ...@@ -15,8 +15,8 @@ instrument code:
* `instrument_instance_method`: instruments a single instance method. * `instrument_instance_method`: instruments a single instance method.
* `instrument_class_hierarchy`: given a Class this method will recursively * `instrument_class_hierarchy`: given a Class this method will recursively
instrument all sub-classes (both class and instance methods). instrument all sub-classes (both class and instance methods).
* `instrument_methods`: instruments all public class methods of a Module. * `instrument_methods`: instruments all public and private class methods of a Module.
* `instrument_instance_methods`: instruments all public instance methods of a * `instrument_instance_methods`: instruments all public and private instance methods of a
Module. Module.
To remove the need for typing the full `Gitlab::Metrics::Instrumentation` To remove the need for typing the full `Gitlab::Metrics::Instrumentation`
......
...@@ -56,7 +56,7 @@ module Gitlab ...@@ -56,7 +56,7 @@ module Gitlab
end end
end end
# Instruments all public methods of a module. # Instruments all public and private methods of a module.
# #
# This method optionally takes a block that can be used to determine if a # This method optionally takes a block that can be used to determine if a
# method should be instrumented or not. The block is passed the receiving # method should be instrumented or not. The block is passed the receiving
...@@ -65,7 +65,8 @@ module Gitlab ...@@ -65,7 +65,8 @@ module Gitlab
# #
# mod - The module to instrument. # mod - The module to instrument.
def self.instrument_methods(mod) def self.instrument_methods(mod)
mod.public_methods(false).each do |name| methods = mod.methods(false) + mod.private_methods(false)
methods.each do |name|
method = mod.method(name) method = mod.method(name)
if method.owner == mod.singleton_class if method.owner == mod.singleton_class
...@@ -76,13 +77,14 @@ module Gitlab ...@@ -76,13 +77,14 @@ module Gitlab
end end
end end
# Instruments all public instance methods of a module. # Instruments all public and private instance methods of a module.
# #
# See `instrument_methods` for more information. # See `instrument_methods` for more information.
# #
# mod - The module to instrument. # mod - The module to instrument.
def self.instrument_instance_methods(mod) def self.instrument_instance_methods(mod)
mod.public_instance_methods(false).each do |name| methods = mod.instance_methods(false) + mod.private_instance_methods(false)
methods.each do |name|
method = mod.instance_method(name) method = mod.instance_method(name)
if method.owner == mod if method.owner == mod
......
...@@ -9,9 +9,31 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -9,9 +9,31 @@ describe Gitlab::Metrics::Instrumentation do
text text
end end
class << self
def buzz(text = 'buzz')
text
end
private :buzz
def flaky(text = 'flaky')
text
end
protected :flaky
end
def bar(text = 'bar') def bar(text = 'bar')
text text
end end
def wadus(text = 'wadus')
text
end
private :wadus
def chaf(text = 'chaf')
text
end
protected :chaf
end end
allow(@dummy).to receive(:name).and_return('Dummy') allow(@dummy).to receive(:name).and_return('Dummy')
...@@ -208,6 +230,21 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -208,6 +230,21 @@ describe Gitlab::Metrics::Instrumentation do
described_class.instrument_methods(@dummy) described_class.instrument_methods(@dummy)
expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true) expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true)
expect(@dummy.method(:foo).source_location.first).to match(/instrumentation\.rb/)
end
it 'instruments all protected class methods' do
described_class.instrument_methods(@dummy)
expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true)
expect(@dummy.method(:flaky).source_location.first).to match(/instrumentation\.rb/)
end
it 'instruments all private instance methods' do
described_class.instrument_methods(@dummy)
expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true)
expect(@dummy.method(:buzz).source_location.first).to match(/instrumentation\.rb/)
end end
it 'only instruments methods directly defined in the module' do it 'only instruments methods directly defined in the module' do
...@@ -241,6 +278,21 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -241,6 +278,21 @@ describe Gitlab::Metrics::Instrumentation do
described_class.instrument_instance_methods(@dummy) described_class.instrument_instance_methods(@dummy)
expect(described_class.instrumented?(@dummy)).to eq(true) expect(described_class.instrumented?(@dummy)).to eq(true)
expect(@dummy.new.method(:bar).source_location.first).to match(/instrumentation\.rb/)
end
it 'instruments all protected instance methods' do
described_class.instrument_instance_methods(@dummy)
expect(described_class.instrumented?(@dummy)).to eq(true)
expect(@dummy.new.method(:chaf).source_location.first).to match(/instrumentation\.rb/)
end
it 'instruments all private instance methods' do
described_class.instrument_instance_methods(@dummy)
expect(described_class.instrumented?(@dummy)).to eq(true)
expect(@dummy.new.method(:wadus).source_location.first).to match(/instrumentation\.rb/)
end end
it 'only instruments methods directly defined in the module' do it 'only instruments methods directly defined in the module' do
...@@ -253,7 +305,7 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -253,7 +305,7 @@ describe Gitlab::Metrics::Instrumentation do
described_class.instrument_instance_methods(@dummy) described_class.instrument_instance_methods(@dummy)
expect(@dummy.method_defined?(:_original_kittens)).to eq(false) expect(@dummy.new.method(:kittens).source_location.first).not_to match(/instrumentation\.rb/)
end end
it 'can take a block to determine if a method should be instrumented' do it 'can take a block to determine if a method should be instrumented' do
...@@ -261,7 +313,7 @@ describe Gitlab::Metrics::Instrumentation do ...@@ -261,7 +313,7 @@ describe Gitlab::Metrics::Instrumentation do
false false
end end
expect(@dummy.method_defined?(:_original_bar)).to eq(false) expect(@dummy.new.method(:bar).source_location.first).not_to match(/instrumentation\.rb/)
end end
end end
end end
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