Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrument Reflex processing in development and log a summary #686

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Matt-Yorkley
Copy link
Contributor

Enhancement

Description

Tracks time spent processing a Reflex and spits out a summary in the logs (in development) on completion, eg:

Processed LikeReflex#like in 28.8ms (Views: 5.4ms | ActiveRecord: 9.3ms | Allocations: 13497)

Why should this be added

It's nice to see the summaries when optimising querying / rendering.

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

Before (wall of text):

Screenshot from 2024-02-24 16-11-57

After (summaries and newline):

Screenshot from 2024-02-24 16-11-20

Copy link

netlify bot commented Feb 24, 2024

Deploy Preview for stimulusreflex ready!

Name Link
🔨 Latest commit 876bf7c
🔍 Latest deploy log https://app.netlify.com/sites/stimulusreflex/deploys/6600385b9c9b400008434b87
😎 Deploy Preview https://deploy-preview-686--stimulusreflex.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Matt-Yorkley Matt-Yorkley force-pushed the instrumented branch 3 times, most recently from 39407a0 to cb32fb5 Compare February 24, 2024 17:24
@marcoroth
Copy link
Member

Thanks for opening this @Matt-Yorkley, this is great! Would you mind resolving the Standard errors?

@marcoroth
Copy link
Member

I just ran this in my app and encountered this error when calling a reflex, do you have a hunch what this could be about?

Reflex FolderReflex#edit failed: wrong number of arguments (given 5, expected 1) [http://localhost:3000/]
/Users/marcoroth/Development/stimulus_reflex/lib/stimulus_reflex/instrumentation.rb:11:in `block (2 levels) in track'
/Users/marcoroth/.anyenv/envs/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activesupport-6.1.7.7/lib/active_support/notifications/fanout.rb:186:in `finish'
/Users/marcoroth/.anyenv/envs/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activesupport-6.1.7.7/lib/active_support/notifications/fanout.rb:63:in `block in finish'
/Users/marcoroth/.anyenv/envs/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activesupport-6.1.7.7/lib/active_support/notifications/fanout.rb:63:in `each'
/Users/marcoroth/.anyenv/envs/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activesupport-6.1.7.7/lib/active_support/notifications/fanout.rb:63:in `finish'
/Users/marcoroth/.anyenv/envs/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activesupport-6.1.7.7/lib/active_support/notifications/instrumenter.rb:45:in `finish_with_state'
/Users/marcoroth/.anyenv/envs/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activesupport-6.1.7.7/lib/active_support/notifications/instrumenter.rb:30:in `instrument'
/Users/marcoroth/.anyenv/envs/rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/activerecord-6.1.7.7/lib/active_record/connection_adapters/abstract_adapter.rb:687:in `log'

@Matt-Yorkley
Copy link
Contributor Author

Yikes! First thought from eyeballing the stacktrace; is the public interface for Benchmark.realtime is different between Ruby versions? Something there is expecting a different number of arguments.

I'll take a look...

@Matt-Yorkley
Copy link
Contributor Author

It's related to changes in ActiveSupport::Notifications between Rails versions... it works fine in 7.1.3 😞

@Matt-Yorkley
Copy link
Contributor Author

I just tried a quick dirty-downgrade to Rails 6 to check this out and it looks like if the callback passed to ActiveSupport::Notifications.subscribed is a Lambda in Rails 6 it doesn't like it(?), but changing it to a Proc fixes the issue and also works fine in 7.0 and 7.1.

@marcoroth
Copy link
Member

Thanks for giving this a look @Matt-Yorkley! Seems like that did the trick!

The only other thing I noticed is that the log entries can appear out-of-order. But, I'm not sure we can actually do something about this:

CleanShot 2024-03-12 at 05 08 33

Do you see a way how we could resolve this? If not, we can probably merge this as-is.

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Mar 12, 2024

Are you doing a page morph? It looks like the reflex executes, then a controller instance is handling a page render, then the broadcast is going out via ActionCable?

Might be getting a bit philosophical here, but is that order even incorrect?

@Matt-Yorkley Matt-Yorkley force-pushed the instrumented branch 3 times, most recently from b0342b2 to ac61a2a Compare March 24, 2024 00:34
It turns out emitted `render` events from Rails overlap cumulatively if you render a partial in a partial, and there's no nice way to differentiate parent partials from children, so the duration and allocations counts can be way off when the totals from the events are summed. View time can end up being twice as much as the total time, which is obviously impossible. Rails cheats a bit for it's rendering totals by taking the whole page template, but that doesn't work for SR, so allocations and rendering time have to be calculated a bit differently.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants