-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Add allocations to template renderer subscription #34136
Conversation
69a33bc
to
0571e18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow thanks @eileencodes! I just added a nitpicky comment 🔍
@@ -20,12 +20,15 @@ def process_action(event) | |||
info do | |||
payload = event.payload | |||
additions = ActionController::Base.log_process_action(payload) | |||
|
|||
status = payload[:status] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: After removing the blank line I would format status
to the same level asadditions
as we seem to follow that style across this file.
payload = event.payload
additions = ActionController::Base.log_process_action(payload)
status = payload[:status]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went the other way. Evening the space makes searching harder so I'm not a huge fan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for caring about this and for the PR ❤️
This PR adds the allocations to the instrumentation for template and partial rendering. Before: ``` Rendering posts/new.html.erb within layouts/application Rendered posts/_form.html.erb (9.7ms) Rendered posts/new.html.erb within layouts/application (10.9ms) Completed 200 OK in 902ms (Views: 890.8ms | ActiveRecord: 0.8ms) ``` After: ``` Rendering posts/new.html.erb within layouts/application Rendered posts/_form.html.erb (Duration: 7.1ms | Allocations: 6004) Rendered posts/new.html.erb within layouts/application (Duration: 8.3ms | Allocations: 6654) Completed 200 OK in 858ms (Views: 848.4ms | ActiveRecord: 0.4ms | Allocations: 1539564) ```
0571e18
to
e8c1be4
Compare
Since this uses GC.stat, do we care that this will throw out potentially nonsense numbers for multithreaded application servers like Puma? Should this be turned off or somehow warning-flagged in those environments? |
@tenderlove @eileencodes forgot to tag. |
This PR adds the allocations to the instrumentation for template and
partial rendering.
Before:
After:
cc/ @tenderlove