app_profiler icon indicating copy to clipboard operation
app_profiler copied to clipboard

Make profiler backend modular, add support for vernier profiler

Open dalehamel opened this issue 2 years ago • 4 comments

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, Profile is now an abstract / mostly virtual base class, with StackprofProfile child class replacing the previous functionality of Profile. A new VernierProfile is also a child class.
    • This was necessary as Vernier returns a Result object, not a hash.
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::VernierBackend OR specifying backend=vernier via 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.

dalehamel avatar Oct 19 '23 22:10 dalehamel

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)

dalehamel avatar Feb 05 '24 19:02 dalehamel

@gmcgibbon I believe I've addressed all the comments from our pairing session, please take another look when you have a chance ❤️

dalehamel avatar Feb 08 '24 22:02 dalehamel

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!

dalehamel avatar Feb 20 '24 21:02 dalehamel

@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.

dalehamel avatar Mar 08 '24 19:03 dalehamel

@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 😓

dalehamel avatar Apr 30 '24 19:04 dalehamel

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...

dalehamel avatar Apr 30 '24 19:04 dalehamel

Ok I've got it back to green by:

  • Tricking older rubies into using a separate lockfile in CI using BUNDLE_GEMFILE and 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

dalehamel avatar Apr 30 '24 20:04 dalehamel

@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.

dalehamel avatar May 01 '24 15:05 dalehamel