-
Notifications
You must be signed in to change notification settings - Fork 949
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 live_component update telemetry event #2914
Add live_component update telemetry event #2914
Conversation
4d05ee5
to
5bb31c3
Compare
lib/phoenix_live_view/diff.ex
Outdated
cid: cid, | ||
new?: new? | ||
} | ||
end) |
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.
This code can be very performance sensitive. Doing all of these traversals for telemetry purposes is going to be expensive. We can start with a single telemetry passing :component
and :assigns_sockets
as metadata.
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.
Fair enough, I thought about passing metadata (the internal "variable") , but that would probably be leaking an internal structure, so I did this.
I'll change to just pass the assigns_sockets. On the stop event, do you think I should replace sockets or just add a new metadata field with the new sockets?
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.
Just return the new sockets and the component, yeah.
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.
Done 😄
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.
Can you please revert the other changes to the file and keep it a simple wrapper around the existing calls?
But the refactor of making assigns_sockets is necessary, otherwise the render would be happening in the the inner loop and we would not be able to send assigns_sockets in the telemetry metadata. Also arguably it is a little bit better IMHO since before when no update_many was defined, the name I can revert other small changes, but this is already the bulk of the changes. |
I'll try to remove as much of unnecessary changes as possible, though. |
I see, that makes sense. Thank you! 💚 💙 💜 💛 ❤️ |
The second part of what was discussed on this thread. This together with #2907 will already give good hooks for traces to be able to correlate nested component renders.
For this one, I decided to group cid, socket, assigns and whether the component is new or not in a list of maps to make life easier for whoever is consuming this.
I took the opportunity to do a slightly refactor to make the "shape" of
assigns_sockets
uniform regardless of whether the component haveupdate_many
exported or not. This was done simply by just adding{new_assigns, socket}
to the assigns_sockets in the inner reduce and then calling update after inside amap
call.