Make profiler backend modular, add support for vernier profiler
This aims to add support for the new vernier ruby profiler.
To facilitate this, It was necessary to make two fairly major structural changes:
- The "Profiler" now has two possible backends, "Stackprof" (the default) and "Vernier
- This can be overridden by setting AppProfiler.backend
- The vernier backend is not yet fully featured
graph TD;
Profiler-->Backend-->StackprofBackend;
Profiler-->Backend-->VernierBackend;
- Rather than generic instances of
Profile,Profileis now an abstract / mostly virtual base class, withStackprofProfilechild class replacing the previous functionality ofProfile. A newVernierProfileis also a child class.- This was necessary as Vernier returns a
Resultobject, not a hash.
- This was necessary as Vernier returns a
graph TD;
Profile-->StackprofProfile;
Profile-->VernierProfile;
The size of the change appears larger than it is because some files were moved during the refactoring, but changed enough that they appear as new files - the stackprof related code and tests should be more or less unchanged.
New behaviour
Stackprof remains the default backend. On older rubies that don't support vernier, vernier will not be possible to use.
On ruby 3.2.1+, it will be possible to use the vernier backend by:
- Separately adding vernier to the application gem file
- NB It is not a gem dependency of app_profiler due to the ruby version constraints
- Explicitly setting the backend to vernier with
Stackprof.backend = AppProfiler::VernierBackendOR specifyingbackend=verniervia the middleware (either via query param, or header). - Specifying the "with_backend" param when calling
AppProfiler.run, eg:AppProfiler.run(with_backend: AppProfiler::VernierBackend)
Note that vernier does not support CPU or object mode profiles, but does support wall and a new type of profile, "retained" memory.
It is not possible to concurrently use vernier and stackprof, as this is unsafe and will result in segfaults. A mutex is introduced to AppProfiler.run to prevent this.
🎩
To tophat this, it is probably best to use https://github.com/Shopify/app_profiler/pull/111 which provides a means of viewing the new vernier profiles.
Otherwise on its own this provides a means to collect vernier profiles but just not to view them.
Notes from pairing:
- [x] See if we can push the run lock into the the concrete backends' .start (eg, vernier.start, stackprof.start), and release in stop. Currently only AppProfiler.run is protected
- [x] Move vernier backend to using autoloading and not require
- [x] Store vernier profiles as the gecko hash / json to be similar to stackprof, not the result object.
- [x] Public change: Profile is now abstract, rename to AbstractProfile, have deprecation notice for usage of Profile (see https://api.rubyonrails.org/classes/ActiveSupport/Deprecation/DeprecatedConstantAccessor.html#method-i-deprecate_constant)
@gmcgibbon I believe I've addressed all the comments from our pairing session, please take another look when you have a chance ❤️
Does that happen in a rails app? Check to make sure the deprecation behaviour in the application is set to log, if you want it to log. I think the fact that it was raising is dependent on the app's config.
Added more details in https://github.com/Shopify/app_profiler/pull/101#discussion_r1496535200 EDIT - looks like if i specify the full namespace, it works.
Thanks for your great review @gmcgibbon , sorry it took me a week to get back to it but I was sick all of last week. I believe I've addressed all your comments either with code or additional clarifying comments, and I added some clarity around the deprecation issues I am having.
Please take a look when you have another chance - thanks!
@gmcgibbon I believe I addressed your most recent batch of comments, please take another look when you have a chance. Sorry for the long delay on the back and forth again, I had some vacation that prevented as quick of a turn-around as I would have liked.
@gmcgibbon I think i've done the renames and moves as you requested, and I replied to your two other suggestions.
The CI bundle cache from main seems to have borked somehow, which is why everything but ruby 3.2 (introduced here) is failing. Not sure how best to fix that, but that is why there is red in the tests. I guess I'll take a look at that next to try and get this to green 😓
Ah the failures seem legit, I guess we need something like https://github.com/Shopify/app_profiler/pull/101#discussion_r1471901897 - one for the older rubies, one for the newer ones...
Ok I've got it back to green by:
- Tricking older rubies into using a separate lockfile in CI using
BUNDLE_GEMFILEand the test matrix - Explicitly splatting an empty params hash in a test, or else ruby 2.7 is sad as the expectation isn't satisfied
@gmcgibbon ok I think I've addressed your latest round of feedback:
- AppProfiler.clear now has a regression test to prevent leaking the backend, and has been made private
- Rather than a symlink, entirely separate gemfiles are used (note that locally, if not working on the latest, you need to explicitly set BUNDLE_GEMFILE now)
- I added a conditional and a comment around the ruby 2.7 expectation / param splat quirk.