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

Better logging of cached partial renders #23879

Closed
dhh opened this Issue Feb 25, 2016 · 19 comments

Comments

Projects
None yet
5 participants
@dhh
Member

dhh commented Feb 25, 2016

Now that we have the lovely [X/Y cache hits] formats for partial collections, I think we should expand that to single partial renders as well.

Here's what things currently look like:

Read fragment views/v1/2914079/v1/2914079/recordings/70182313-20160225015037000000/d0bdf2974e1ef6d31685c3b392ad0b74 (0.6ms)
Rendered messages/_message.html.erb in 1.2 ms
Write fragment views/v1/2914079/v1/2914079/recordings/70182313-20160225015037000000/3b4e249ac9d168c617e32e84b99218b5 (1.1ms)
Rendered recordings/threads/_thread.html.erb in 1.5 ms

I'd like to see this instead:

Rendered messages/_message.html.erb in 1.2 ms [cache hit]
Rendered recordings/threads/_thread.html.erb in 1.5 ms [cache miss]

@dhh dhh added the actionview label Feb 25, 2016

@dhh dhh added this to the 5.1.0 milestone Feb 25, 2016

@st0012

This comment has been minimized.

Contributor

st0012 commented Feb 25, 2016

I'm working on this 😄

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 25, 2016

@st0012 Neat! But mind the milestone, I won't have much time to review a pull request before Rails 5.0 is out.

@st0012

This comment has been minimized.

Contributor

st0012 commented Feb 25, 2016

@kaspth ok, but I will still mention you when I done this.

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 25, 2016

@st0012 by all means do! I'm just setting expectations 😁

@st0012

This comment has been minimized.

Contributor

st0012 commented Mar 22, 2016

@kaspth Hi, I noticed that collection partials' caching functionality and instrument option was implemented here, but partial rendering doesn't have something like that. So does this issue also require implementing partial caching mechanism?

@kaspth

This comment has been minimized.

Member

kaspth commented Mar 22, 2016

No, there's already cache in views for that. We need to make that able to fill in :cache_hit in the instrumentation payload like we do for collection caching 😁

@st0012

This comment has been minimized.

Contributor

st0012 commented Mar 22, 2016

So that means we need to shift the instrumenting to earlier actions, like cache in caching_helper?

@st0012

This comment has been minimized.

Contributor

st0012 commented Mar 22, 2016

Oh, fragment_for might be a better place to do that.

@kaspth

This comment has been minimized.

Member

kaspth commented Mar 22, 2016

@st0012 try researching it a bit more and write something more substantial. This isn't a chat app 😁

@st0012

This comment has been minimized.

Contributor

st0012 commented Mar 22, 2016

@kaspth Sorry. I already started working on that today, I just curious about what approach to take (and it seems that I misunderstood something). I hope I can open a PR before the end of this week.

@kaspth

This comment has been minimized.

Member

kaspth commented Mar 22, 2016

No worries, and don't worry about getting the PR ready for the end of the week. We can't merge this before the Rails 5 stable branch is created.

@st0012

This comment has been minimized.

Contributor

st0012 commented Mar 23, 2016

After some research, here's what I found:
The reason collection partials caching can be instrumented is because the cached option would be passed in render method, like

<%= render partial: 'notifications/notification', collection: @notifications, cached: true %>

And the collection partial's caching is implemented in the partial rendering level. So the Logsubscriber knows that whether the collections should be cached and whether cache is success. The process may looks like this:

PartialRenderer#render_collection
PartialRenderer::CollectionCaching#cache_collection_render (pass payload here)
(Caching collections and put result into payload)
Done

However, normal partial caching happens in the last step of view rendering, which is process the template content. That means we can't know if the view uses cache nor it's cache result, unless we figure out some way to pass the payload deep into that level, but that will touch a lot of things I think.

@kaspth

This comment has been minimized.

Member

kaspth commented Mar 23, 2016

That wasn't really what I meant by research. This is what you should look into:

That means we can't know if the view uses cache nor it's cache result, unless we figure out some way to pass the payload deep into that level, but that will touch a lot of things I think.

What we can take from the collection caching stuff is how it fills in the instrumentation payload as it's running, that's what we need to do for normal partial renders too 😁

@rails-bot rails-bot added the stale label Jul 13, 2016

@rails-bot

This comment has been minimized.

rails-bot commented Jul 13, 2016

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 5-0-stable branch or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@kaspth kaspth added pinned and removed stale labels Jul 13, 2016

@kaspth

This comment has been minimized.

Member

kaspth commented Jul 13, 2016

@st0012 hey, how far did you come with this? I'm curious to see what you got 😁

@st0012

This comment has been minimized.

Contributor

st0012 commented Jul 14, 2016

@kaspth Hi, sorry that I just forgot this issue for last few months. I just restudied it after Rails 5 officially came out. 😓
I think I know what to do with this issue after restudied the codebase and our conversation, and I will do some naive implementation this weekend and attach a PR this weekend.

@st0012

This comment has been minimized.

Contributor

st0012 commented Jul 14, 2016

@kaspth I just attached my PR, hope that you can take a look and give me some advice 😁

@st0012

This comment has been minimized.

Contributor

st0012 commented Aug 8, 2016

@kaspth I thinks this can be closed?

@kaspth

This comment has been minimized.

Member

kaspth commented Aug 8, 2016

@st0012 right you are! I'm too used to pull requests having "Fixes issue-number" and GitHub closing issues automatically that I forget it's not always so. That's a tip for your next pull request and thanks 😁

Fixed in #25825.

@kaspth kaspth closed this Aug 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment