Instrument private methods and instance private methods
What does this MR do?
Instrument private methods and instance methods by default not just public methods
Why was this MR needed?
To have more visibility of the performance of the code
What are the relevant issue numbers?
Does this MR meet the acceptance criteria?
-
CHANGELOG entry added -
Documentation created/updated [ ] API support added-
Tests -
Added for this feature/bug -
All builds are passing
-
-
Conform by the style guides -
Branch has no merge conflicts with master(if you do - rebase it please) -
Squashed related commits together
Merge request reports
Activity
11 11 12 class << self 13 def buzz(text = 'buzz') 14 text 15 end 16 private :buzz 17 end 18 12 19 def bar(text = 'bar') 13 20 text 14 21 end 22 23 def wadus(text = 'wadus') 24 text 25 end 26 private :wadus Reassigned to @yorickpeterse
65 65 # 66 66 # mod - The module to instrument. 67 67 def self.instrument_methods(mod) 68 mod.public_methods(false).each do |name| 68 methods = mod.public_methods(false) + mod.private_methods(false) 69 methods.each do |name| @pacoguzman: Can we not just use
mod.methods(false)in this case? I think that also includes protected methods but it's nice to have those instrumented as well.Edited by yorickpeterse-staging@pacoguzman: Ah yes,
methodsdoesn't include private methods apparently. Go for it :)
253 281 254 282 described_class.instrument_instance_methods(@dummy) 255 283 256 expect(@dummy.method_defined?(:_original_kittens)).to eq(false) 284 expect(@dummy.new.method(:kittens).source_location.first).not_to match(/instrumentation\.rb/) I think that was a previous implementation that maybe was doing alias_method or something like that. If you check the specs that asserts the method is instrumented are not checking that
:_original_fooor whatever is defined is just checking that the proxy_module is inserted, that's why I decided to do this way so we assert in the same way both cases (when the method is instrumented and when is not instrumented).Maybe here instead using
not_tomaybe we want to usetoand match about the location which is this current file.@pacoguzman: Ah yes correct, the aliases are no longer used. In that case using the source location is probably the best way to go.
@pacoguzman: Ah yes you're right, the aliasing is no longer used. In that case using the source location is probably the best way of doing it.
Reassigned to @pacoguzman
Milestone changed to %8.9
Added 21 commits:
- db267f21...0c0ef7df - 20 commits from branch
master - 4da191c5 - Instrument private/protected methods
- db267f21...0c0ef7df - 20 commits from branch
Reassigned to @yorickpeterse
Some of my notes appear to be disappearing, so I'll leave the last one here regarding the aliasing test:
@pacoguzman: Ah yes you're right, the aliasing is no longer used. In that case using the source location is probably the best way of doing it.
@yorickpeterse work done! and build is green!
@pacoguzman: There's a merge conflict in
doc/development/instrumentation.mdwhich seems to be caused by your MR to add CPU timings. Could you rebase and resolve the conflict? Other than that everything looks fine.Reassigned to @pacoguzman
@yorickpeterse build green and without conflicts
