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

Adding process tracing #165

Merged

Conversation

akoutmos
Copy link
Contributor

@akoutmos akoutmos commented Jul 2, 2022

This PR implements #159

Continuing the discussion from that issue:

Should we not show the bars if the process already exists or should we show the bar from beginning to end?

At the moment I have the bar starting when a process is spawned and stopped when it exits (both of which have to be present during the trace in order for the bar to appear). In other words, it doesn't look like Mermaid renders anything unless there is both an actor activate and deactivate. That and it doesn't look like we can start bars at arbitrary points on the sequence. Rather it looks like they have to start and stop on events. All this to say, I think if we want to keep the bars, they'll only be present when a process starts and stops during the trace.

Regarding the messages, can we do any sort of mouse over thing? Keep only the label and when you mouse over, you see the whole message?

I'll have to explore this a bit more. I tried setting up a pop up Actor Menu as described in the docs...but the menu never popped up lol.

There are some dotted arrows after the messages... is this the "receive" event? If so, I think we can ignore it. Another option would be for the arrows to not be horizontal, but point to when the messages are received in the other process timeline, but that may be too complicated?

I think the Mermaid diagrams only allow for horizontal lines and cannot be linked back to another event. Do you want me to remove all receive events and just have send events? Or only in certain circumstances? Here is an example with no receives for reference:

image

We also need to check if we trace :DOWN messages from Process.monitor.

It has been done! I also added that to the example code :).

I have a couple more things that I need to clean up and fix so I am opening this up as a draft PR. Should have those wrapped up during the weekend :). Thanks in advance for the review and have a great weekend!

lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
@josevalim
Copy link
Contributor

@jonatanklosko the current implementation discards the result of the function given to trace and I don't like that. We could instead return {res, kino_markdown}, but then the result of course won't be printed. So here is my suggestion:

  1. Have this function return both {res, kino_markdown}
  2. Also add Kino.Process.render_trace, that calls trace/2, renders the trace behind the scenes, and returns only res

WDYT?

Copy link
Contributor

@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.

Amazing @akoutmos! I have added only some minor final comments!

Do you want me to remove all receive events and just have send events?

Yes, I think we can remove them all. :)

akoutmos and others added 7 commits July 2, 2022 11:03
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
akoutmos and others added 2 commits July 2, 2022 11:26
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
akoutmos and others added 3 commits July 2, 2022 11:29
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
@akoutmos
Copy link
Contributor Author

akoutmos commented Jul 2, 2022

Thanks for the review @josevalim! Really appreciate it. I reworked the exception handling a little so that the call to the tracer GenServer was not part of the trace capture. I also need to rework that last reduce to handle the scenario when a process is started prior to the trace, but terminated during the trace. Mermaid throws an error if you attempt to deactivate an actor without it being activated prior.

After that I think it is good to go :). Thanks!

akoutmos and others added 2 commits July 2, 2022 11:43
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
@josevalim
Copy link
Contributor

You can also do nested trys. The nesting is uglier but less code duplication and perhaps easier to understand?

try do
  try do
    fun.()
  after
    stop_trace
  end

  get_trace_events
after
  GenServer.stop
end

@akoutmos
Copy link
Contributor Author

akoutmos commented Jul 2, 2022

You can also do nested trys. The nesting is uglier but less code duplication and perhaps easier to understand?

try do
  try do
    fun.()
  after
    stop_trace
  end

  get_trace_events
after
  GenServer.stop
end

It has been done :)

@akoutmos akoutmos marked this pull request as ready for review July 2, 2022 17:16
@akoutmos
Copy link
Contributor Author

akoutmos commented Jul 2, 2022

I fixed that last issue that was on my TODO list and switched the PR from draft to ready. Let me know if there are any other issues I should address.

I think the only open question at the moment is whether the result is kept from the trace function?

@josevalim
Copy link
Contributor

I will let you know soon. I also just realized that we could name the current process as self() in the diagram. WDYT?

@akoutmos
Copy link
Contributor Author

akoutmos commented Jul 3, 2022

I will let you know soon. I also just realized that we could name the current process as self() in the diagram. WDYT?

Sounds good :). I updated how the participating PIDs are generated and the calling PID now show up as self(). I think that makes it nice and clear for users following the trace:

image

@josevalim
Copy link
Contributor

Hi @akoutmos, let's please change this one to return {res, kino}. Then we will add render_app_tree, render_sup_tree and render_seq_trace that render as a side-effect. The first two will return Kino.nothing, the last one will return the result of the function. WDYT? Can you please push those final changes? :)

Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

@akoutmos fantastic!!

lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process.ex Outdated Show resolved Hide resolved
lib/kino/process/tracer.ex Outdated Show resolved Hide resolved
lib/kino/process/tracer.ex Outdated Show resolved Hide resolved
lib/kino/process/tracer.ex Outdated Show resolved Hide resolved
akoutmos and others added 5 commits July 3, 2022 22:33
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
@akoutmos
Copy link
Contributor Author

akoutmos commented Jul 4, 2022

@akoutmos fantastic!!

Thanks @jonatanklosko!

@akoutmos
Copy link
Contributor Author

akoutmos commented Jul 4, 2022

Hi @akoutmos, let's please change this one to return {res, kino}. Then we will add render_app_tree, render_sup_tree and render_seq_trace that render as a side-effect. The first two will return Kino.nothing, the last one will return the result of the function. WDYT? Can you please push those final changes? :)

That sounds good to me @josevalim :). I think I implemented it as you described in 9a5b965. Let me know what you think!

@josevalim josevalim merged commit e5b3261 into livebook-dev:main Jul 4, 2022
@josevalim
Copy link
Contributor

💚 💙 💜 💛 ❤️

@akoutmos
Copy link
Contributor Author

akoutmos commented Jul 4, 2022

Thanks for the review and the help @josevalim and @jonatanklosko! Happy to see this merged in :)

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.

None yet

3 participants