livebook-dev / kino

Client-driven interactive widgets for Livebook
Apache License 2.0
361 stars 60 forks source link

Add ETS table rendering to Kino.Process.{sup_tree,app_tree} #430

Closed akoutmos closed 3 months ago

akoutmos commented 3 months ago

While working on the Elixir Patterns book I found it useful to include ETS tables in the Kino.Process.sup_tree/1 rendering so readers could see the layout of process as well as any ETS tables associated with those processes. Something like so:

image

If there is interest in adding this to Kino I can open up a PR with my work :).

josevalim commented 3 months ago

This looks nice!

Two questions:

  1. how can both processes be associated to an ETS table? Usually we have one as owner? Is that the heir?
  2. maybe the table should be inside the process box?

You can also try it out in a Registry to see how it looks, as it has two ETS tables.

akoutmos commented 3 months ago

Thanks!

  1. Correct. The solid line to the ETS table is the owning process and the dotted line is the heir process.
  2. good idea....I didn't even think about that. Do you picture it using Mermaid subgraphs?
josevalim commented 3 months ago

Correct. The solid line to the ETS table is the owning process and the dotted line is the heir process.

I would invert the arrow then (perhaps annotate it with (heir) if possible.

good idea....I didn't even think about that. Do you picture it using Mermaid subgraphs?

That would be the idea but I am not sure if it is better in practice.

akoutmos commented 3 months ago

Sounds good. I'll put together a couple hard coded mocks and post them here with which route we want to go.

akoutmos commented 3 months ago

Here are some hardcoded Mermaid diagrams for how we can render this:

Diagram without subgraph:

image

Diagram with subgraph:

image

We can also drop the arrows all together for ETS tables and label both owner and heir to be explicit

image

Thoughts?

josevalim commented 3 months ago

I think the original one is less cluttered. I would only add an arrow to heir, but pointing to the process instead. What are your thoughts? Any thoughts @hugobarauna?

akoutmos commented 3 months ago

When the direction of the ETS owner edge changes, Mermaid shuffles the graph in a not so intuitive way:

image
hugobarauna commented 3 months ago

I think the original one is less cluttered. I would only add an arrow to heir, but pointing to the process instead. What are your thoughts? Any thoughts @hugobarauna?

I think the original one is less cluttered.

Agree, without subgraphs.

I would only add an arrow to heir

From an educational point of view, maybe adding both "heir" and "owner" labels is a good direction.

For example, before that discussion, I didn't know an ETS table could have a relationship with two processes; I thought it was only related to one process. I just learned about the "heir" thing.

So, for someone like me (who is learning about ETS), if the diagram shows both labels, it can help me understand that an ETS table can have a relationship with two processes, one of which is the owner and the other the heir.

I'd keep both labels.

Regarding the arrows, the supervision diagram the way it is already has some implied semantics:

So, maybe we should use a different line from those two to imply a new semantic. Maybe just a simple line without arrows but with the "owner" or "heir" label.

josevalim commented 3 months ago

I would only add an arrow to heir

Sorry, I was not clear. What I meant to say is that I would add an arrow to heir in addition to the label. We should have arrows and labels in both. The reason I like the arrow in the heir case is because the "inheritance" happens by literally sending a message which transfers the table to the heir. But before the owner dies, nothing ever happens. The arrow in ownership is also important to say who owns who. For someone who is new, those relationships may not be clear, so I would emphasize them with arrows.

akoutmos commented 3 months ago

I think I got it lol. Reread all the comments a couple times and I'm pretty sure this is what you are both looking for (I also added the table access inside the DB diagram):

image
josevalim commented 3 months ago

Sorry, the heir is from the table to the process. Everything else is perfect. :) Should we make the display of ETS tables opt-in or opt-out? Probably opt-in to avoid initial clutter?

akoutmos commented 3 months ago

Sounds good. Pretty sure I have everything as discussed in PR https://github.com/livebook-dev/kino/pull/432

akoutmos commented 3 months ago

Closing this as #432 was merged.