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

Custom Label Logic #411

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Conversation

mariari
Copy link
Contributor

@mariari mariari commented Mar 28, 2024

This topic addresses #408

I added an extra argument to seq_trace and render_seq_trace, that accepts an extra argument to augment the labeling logic.

I have not tested the code yet, nor did I see any tests in the repo.

I hope it works.

I am now going to test this in a project I'm working on, and might add an extra commit that augments the example, showing off what the custom label can do.

I've also exposed label_from_value as I think it may add some value to people's custom's logic's

  @spec foo(term()) :: label_response()
  defp foo(message) do
    case message do
      {:"$gen_call", _ref, {:special, _, msg}} -> "CALL: #{label_from_value(msg)}"
      {:"$gen_cast", _ref, {:special, _, msg}} -> "CAST: #{label_from_value(msg)}"
      _ -> :continue
    end
  end

is one such logic I used in mocking up what I wanted to do. I will get a better example as I make more examples.

@mariari
Copy link
Contributor Author

mariari commented Mar 28, 2024

The code seems to work!

@@ -224,6 +225,14 @@ defmodule Kino.Process do
target argument can either be a single PID, a list of PIDs, or the atom `:all`
depending on what messages you would like to retain in your trace.

## Options

* `:label_function` - A function to help label message events. If
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call it :message_label? We usually don't append the type to the option names.

I also don't think we should expose label_from_value. If you pass a custom function, you usually know better which values you are going to handle, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we call it :message_label? We usually don't append the type to the option names.

Sure, let me change that now.

I also don't think we should expose label_from_value. If you pass a custom function, you usually know better which values you are going to handle, no?

Most likely, I just wanted to preserve the initial message, as the code I'll be running it on just wraps the normal GenServer.call and GenServer.cast with some extra information to the given GenServer, meaning the default behavior is close to what I wanted but not quite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both complaints should be addressed now on the latest push

Namely we add message labeling to the code, so that a user can
override the default labeling logic
We add documentation on how this could be used
@mariari
Copy link
Contributor Author

mariari commented Mar 28, 2024

I have also provided an example in the documentation now on how to run the new message_label option

lib/kino/process.ex Outdated Show resolved Hide resolved
@josevalim josevalim merged commit 6e12f05 into livebook-dev:main Mar 28, 2024
1 check passed
@josevalim
Copy link
Contributor

💚 💙 💜 💛 ❤️

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