Merge branch 'respect_visiblity_instrument_methods' into 'master'

Respect original visibility for instrumented methods

See merge request gitlab-org/gitlab!39951

changelogs/unreleased/respect_visiblity_instrument_methods.yml

+---
+title: Respect original visibility for instrumented methods
+merge_request: 39951
+author:
+type: fixed

lib/gitlab/metrics/instrumentation.rb

@@ -120,9 +120,6 @@ module Gitlab
       def self.instrument(type, mod, name)
         return unless ::Gitlab::Metrics.enabled?
 
-        name = name.to_sym
-        target = type == :instance ? mod : mod.singleton_class
-
         if type == :instance
           target = mod
           method_name = "##{name}"
@@ -154,6 +151,8 @@ module Gitlab
             '*args'
           end
 
+        method_visibility = method_visibility_for(target, name)
+
         proxy_module.class_eval <<-EOF, __FILE__, __LINE__ + 1
           def #{name}(#{args_signature})
             if trans = Gitlab::Metrics::Instrumentation.transaction
@@ -163,11 +162,23 @@ module Gitlab
               super
             end
           end
+          #{method_visibility} :#{name}
         EOF
 
         target.prepend(proxy_module)
       end
 
+      def self.method_visibility_for(mod, name)
+        if mod.private_method_defined?(name)
+          :private
+        elsif mod.protected_method_defined?(name)
+          :protected
+        else
+          :public
+        end
+      end
+      private_class_method :method_visibility_for
+
       # Small layer of indirection to make it easier to stub out the current
       # transaction.
       def self.transaction

spec/lib/gitlab/metrics/instrumentation_spec.rb

@@ -12,6 +12,11 @@ RSpec.describe Gitlab::Metrics::Instrumentation do
         text
       end
 
+      def self.wat(text = 'wat')
+        text
+      end
+      private_class_method :wat
+
       class << self
         def buzz(text = 'buzz')
           text
@@ -242,6 +247,7 @@ RSpec.describe Gitlab::Metrics::Instrumentation do
 
       expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true)
       expect(@dummy.method(:foo).source_location.first).to match(/instrumentation\.rb/)
+      expect(@dummy.public_methods).to include(:foo)
     end
 
     it 'instruments all protected class methods' do
@@ -249,13 +255,16 @@ RSpec.describe Gitlab::Metrics::Instrumentation do
 
       expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true)
       expect(@dummy.method(:flaky).source_location.first).to match(/instrumentation\.rb/)
+      expect(@dummy.protected_methods).to include(:flaky)
     end
 
-    it 'instruments all private instance methods' do
+    it 'instruments all private class 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/)
+      expect(@dummy.private_methods).to include(:buzz)
+      expect(@dummy.private_methods).to include(:wat)
     end
 
     it 'only instruments methods directly defined in the module' do
@@ -290,6 +299,7 @@ RSpec.describe Gitlab::Metrics::Instrumentation do
 
       expect(described_class.instrumented?(@dummy)).to eq(true)
       expect(@dummy.new.method(:bar).source_location.first).to match(/instrumentation\.rb/)
+      expect(@dummy.public_instance_methods).to include(:bar)
     end
 
     it 'instruments all protected instance methods' do
@@ -297,6 +307,7 @@ RSpec.describe Gitlab::Metrics::Instrumentation do
 
       expect(described_class.instrumented?(@dummy)).to eq(true)
       expect(@dummy.new.method(:chaf).source_location.first).to match(/instrumentation\.rb/)
+      expect(@dummy.protected_instance_methods).to include(:chaf)
     end
 
     it 'instruments all private instance methods' do
@@ -304,6 +315,7 @@ RSpec.describe Gitlab::Metrics::Instrumentation do
 
       expect(described_class.instrumented?(@dummy)).to eq(true)
       expect(@dummy.new.method(:wadus).source_location.first).to match(/instrumentation\.rb/)
+      expect(@dummy.private_instance_methods).to include(:wadus)
     end
 
     it 'only instruments methods directly defined in the module' do