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

Add live_component update telemetry event #2914

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

bamorim
Copy link
Contributor

@bamorim bamorim commented Nov 12, 2023

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 have update_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 a map call.

@bamorim bamorim force-pushed the add-update-telemetry-event branch from 4d05ee5 to 5bb31c3 Compare November 12, 2023 15:13
cid: cid,
new?: new?
}
end)
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 😄

Copy link
Member

@josevalim josevalim left a 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?

@bamorim
Copy link
Contributor Author

bamorim commented Nov 13, 2023

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 assigns_sockets was actually just the resulting sockets, so it was not a clear naming.

I can revert other small changes, but this is already the bulk of the changes.

@bamorim
Copy link
Contributor Author

bamorim commented Nov 13, 2023

I'll try to remove as much of unnecessary changes as possible, though.

@josevalim josevalim merged commit 5599a00 into phoenixframework:main Nov 13, 2023
4 checks passed
@josevalim
Copy link
Member

I see, that makes sense. Thank you! 💚 💙 💜 💛 ❤️

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