Filtered process list #411

bokner · 2023-03-02T18:09:59Z

Allow custom implementation of process lists. The motivation is to be able to inspect specific groups of processes rather than pulling the whole process list. For instance, we may want to look at the processes from the specific registry, supervised by a specific supervisor, etc.

josevalim · 2023-03-02T18:25:31Z

Hi @bokner! Thanks for the PR. I think we should simplify this a bit. I think we could have a process_filter and an input. The input defines some text that is sent to the filter. If the filter is enabled, then we call the filter to return a list of processes with the input value.

So the goal is to only filter processes but not customize rendering. WDYT?

bokner · 2023-03-02T19:06:38Z

Hi @josevalim,
Thank you for the quick response. I'm not sure I follow. For purposes of our project, we do want to customize the rendering of the process info page. Specifically, we want to pull some specific data from the registry and show it along with some process_info data (removing some pieces that are not relevant, such as registered_name).

I also realized that more work needs to be done to sync up my code with the latest changes. I started out forking v0.7.1, and I see that things changed quite a bit.

josevalim · 2023-03-02T19:31:04Z

I see. So in this case, I don't think it is a filter, is it? You want to argument the information. Perhaps we should have a ProcessInfo behaviour but it has to return a keyword list of data, instead of rendering on its own.

bokner · 2023-03-02T19:47:44Z

But it is a filter :-) Perhaps, the example of implementation would explain the intention better(sorry, I should have thought about it!):

defmodule ProcessFilter.Demo do
  @behaviour Phoenix.LiveDashboard.ProcessFilter
  def list() do
    ["Registered", "All"]
  end

  def filter("Registered") do
    Process.registered() |> Enum.map(fn name -> %{pid: Process.whereis(name), name_or_initial_call: name, extra: "extra_param"} end)
  end

  def filter("All") do
    Process.list()
  end



  def render_process_info(assigns, filter) when filter in ["Registered"] do
    render(Map.put(assigns, :filter, filter))
  end

  def render_process_info(_assigns, _filter) do
    nil
  end

  use Phoenix.LiveDashboard.Web, :live_component
  def render(assigns) do
    ~H"""
    <div class="tabular-info">
      <%= if @alive do %>
        <table class="table table-hover tabular-info-table">
          <tbody>
            <tr><td class="border-top-0">Filtered by</td><td class="border-top-0"><pre><%= @filter %></pre></td></tr>
            <tr><td class="border-top-0">Registered name</td><td class="border-top-0"><pre><%= @registered_name %></pre></td></tr>
            <tr><td>Total heap size</td><td><pre><%= @total_heap_size %></pre></td></tr>
            <tr><td>Heap size</td><td><pre><%= @heap_size %></pre></td></tr>
            <tr><td>Stack size</td><td><pre><%= @stack_size %></pre></td></tr>
            <tr><td>Reductions</td><td><pre><%= @reductions %></pre></td></tr>
           <tr><td>Current stacktrace</td><td><pre><%= @current_stacktrace %></pre></td></tr>
          </tbody>
        </table>

        <%= if @page.allow_destructive_actions do %>
          <div class="modal-footer">
            <button class="btn btn-danger" phx-target={@myself} phx-click="kill">Kill process</button>
          </div>
        <% end %>
      <% else %>
        <div class="tabular-info-not-exists mt-1 mb-3">Process is not alive or does not exist.</div>
      <% end %>
    </div>
    """
  end
end

So here we have two options for filtering process list, and the process info view for registered processes is customized.

josevalim · 2023-03-02T19:53:38Z

Got it. So I think it is good but instead of render_process_info we should call it extra_process_info and return a keyword list, WDYT?

bokner · 2023-03-02T20:36:49Z

Got it. So I think it is good but instead of render_process_info we should call it extra_process_info and return a keyword list, WDYT?

I like the idea of not dealing with rendering outside of the ProcessInfoComponent, but I'm not sure how do we customize the rendering then? For instance, for some processes, we want to remove registered name and/or some other things, add application-specific links (not just the table rows) etc. Perhaps, we could just provide a list of elements for Phoenix.LiveDashboard.PageBuilder.label_value_list?

josevalim · 2023-03-02T20:41:56Z

So perhaps we may need to return structured data, such as map with key, value and an optional link.

josevalim · 2023-03-02T21:08:25Z

Actually, let’s call it extra_info only, without process, because we can use the same behavior to extend other pages later. :)

…iveView.Rendered, list or nil

bokner · 2023-03-06T18:51:39Z

I think, in addition to returning the content as structured data for the component to render it, we might also want to provide a way to render the content info page directly by the implementation module. The reasoning is that the markup for the info page could be quite complex (as it is for our particular use case), i.e., not limited by the table, basic links, etc.

Then the corresponding callback would be

@callback info_content(assigns :: Socket.assigns(), filter_name :: String.t() | nil) ::
              Phoenix.LiveView.Rendered.t() | [map()] | nil

An example of an implementation that returns the list of maps:

  def info_content(assigns, filter) do
    info_list(assigns)
  end

  defp info_list(assigns) do
    [
      %{label: "Registered name", value: assigns.registered_name},
      %{label: "Total heap size", value: assigns.total_heap_size},
      ## some other pieces
    ]
  end

An example of an implementation that returns the template:

  def info_content(assigns, filter) do
    info_template(assigns)
  end

  defp info_template(assigns) do
    ~H"""
    <table class="table table-hover tabular-info-table">
      <tbody>
        <tr><td class="border-top-0">Registered name</td><td class="border-top-0"><pre><%= @registered_name %></pre></td></tr>
        <tr><td>Total heap size</td><td><pre><%= @total_heap_size %></pre></td></tr>
      </tbody>
    </table>
  """
  end

Both variations are currently handled by Phoenix.LiveDashboard.PageFilter.
What do you think?

josevalim · 2023-03-06T21:01:10Z

Unfortunately for us that is a no-go because we don't want people to assume it is being rendered in a certain way or in a certain part. We need to keep everything driven by Elixir data.

josevalim · 2023-03-06T21:01:45Z

Do you have a screenshot of what you want to render right now? And which kind of rich data would be necessary to achieve it?

alexcastano

I like the idea, it has a lot of potentials, but am unsure how to implement it correctly. I know this PR is WIP, but I left some comments just in case.

About the idea, I think #409 would be a better abstraction for dealing with the custom modal problem. We can detect if it is a "special" process and render a different modal, if not, we can delegate to the regular one.

I'm not 100% sure if I like the filter idea. We already have a good abstraction for this: row_fetcher. Instead of modifying the current behaviour to add more complexity, we could change the row_fetcher "in runtime using a select input" to a custom one. We could expose current helpers for remote execution, order, limit, and filter to simplify this process.

However, all my suggestions would increase the public API making the project harder to maintain in the long term.

Finally, you can always create your own custom page using this PR as a reference.

lib/phoenix/live_dashboard/components/table_component.ex

lib/phoenix/live_dashboard/info/process_info_component.ex

lib/phoenix/live_dashboard/pages/processes_page.ex

lib/phoenix/live_dashboard/info/process_info_component.ex

lib/phoenix/live_dashboard/page_filter.ex

lib/phoenix/live_dashboard/components/table_component.ex

Co-authored-by: Alex Castaño <alexcastanodev@gmail.com>

bokner · 2023-03-07T15:19:53Z

I'm not 100% sure if I like the filter idea. We already have a good abstraction for this: row_fetcher. Instead of modifying the current behaviour to add more complexity, we could change the row_fetcher "in runtime using a select input" to a custom one.

Thank you for the feedback @alexcastano. I'm not sure I completely understand your idea. We would have to introduce some customization either way. Currently, the modification in the original code is limited by the addition of filter parameter to fetch_processes call. Then fetch_processes is using a custom implementation of process list. What would be your suggested strategy here?

alexcastano

Hello again! I've just added some comments to clarify what changes I see as essential before merging :)

lib/phoenix/live_dashboard/page_filter.ex

lib/phoenix/live_dashboard/system_info.ex

lib/phoenix/live_dashboard/components/table_component.ex

lib/phoenix/live_dashboard/page_builder.ex

test/phoenix/live_dashboard/pages/applications_page_test.exs

test/phoenix/live_dashboard/system_info_test.exs

test/phoenix/live_dashboard/pages/processes_live_test.exs

bokner · 2023-04-24T12:57:56Z

@alexcastano

We updated the name from filter to active_filter, so we have to update it here too.

In the code, filter is used as the name of the dropdown, whereas active_filter and filter_list are part of socket.assigns. In particular, active_filter could be updated directly as a result of ProcessFilter module being hot-swapped (or some condition that is coded into the implementation as the polling happens), and not by the user action from UI. So I think it's a good idea to have different names for the form field and the session (i.e., socket.assigns) variable.

bokner · 2023-04-24T13:16:23Z

You have to implement the All filter. In the same way, the user must do it too.
However, we could automatically add All as the first option, which will set active_filter = nil. Here, we can check if active_filter, do: filter_mod.filter(active_filter), else: Process.list(). What do you think?

I would argue that if the implementation has "XXX" in the filter list, it probably does not make sense not to have
a corresponding filter("XXX"). Also, in our particular case, we do not want to have an "All" option for all users, just to avoid accidentally polling millions of processes that we have.

alexcastano · 2023-04-24T20:59:23Z

@alexcastano

We updated the name from filter to active_filter, so we have to update it here too.

In the code, filter is used as the name of the dropdown, whereas active_filter and filter_list are part of socket.assigns. In particular, active_filter could be updated directly as a result of ProcessFilter module being hot-swapped (or some condition that is coded into the implementation as the polling happens), and not by the user action from UI. So I think it's a good idea to have different names for the form field and the session (i.e., socket.assigns) variable.

Sorry, but I don't get it. I don't see any @filter variable used in the template, either socket.assigns.filter in the rest of the code. If this is true, it doesn't make sense to have filter in the specs, in the Map.put_new and the attr. On the other hand, active_filter is used and it could make sense to be modified "externally", for example, using update function.

Maybe are you confused because socket.assigns.table_params.filter is very similar to socket.assigns.filter? I'm just trying to understand :)

alexcastano · 2023-04-24T21:02:52Z

You have to implement the All filter. In the same way, the user must do it too.
However, we could automatically add All as the first option, which will set active_filter = nil. Here, we can check if active_filter, do: filter_mod.filter(active_filter), else: Process.list(). What do you think?

I would argue that if the implementation has "XXX" in the filter list, it probably does not make sense not to have a corresponding filter("XXX"). Also, in our particular case, we do not want to have an "All" option for all users, just to avoid accidentally polling millions of processes that we have.

I don't think it is a problem to have always the "All" option. AFAIK the :observer works in a similar way. Because we have a limit to don't showing every process it should be fine. On the other hand, I was thinking more in a malicious user sending wrong tokens than the developer forgetting to add the clause.

bokner · 2023-04-24T21:30:32Z

I don't think it is a problem to have always the "All" option. AFAIK the :observer works in a similar way.

It is a problem for us, and that's also the reason why we avoid using observer :-)

Edit:

Because we have a limit to don't showing every process it should be fine.

The issue is not so much with how many rows are shown, but rather with server-side polling a potentially huge number of processes and retrieving process info for each of them.

bokner · 2023-04-24T23:06:42Z

Sorry, but I don't get it. I don't see any @filter variable used in the template, either socket.assigns.filter in the rest of the code. If this is true, it doesn't make sense to have filter in the specs, in the Map.put_new and the attr.

You are right, there is none :-)
I will try to convey my understanding of the workflow. I have set up a small application for the test, and the description of the workflow mostly comes from debugging it.

filter is the name of the form field

phoenix_live_dashboard/lib/phoenix/live_dashboard/components/table_component.ex

Line 213 in fe86d97

<select name="filter" class="custom-select" id="filter-select">

The value of this field could be set in 2 ways

By the user selecting the value from the dropdown. This is handled by

phoenix_live_dashboard/lib/phoenix/live_dashboard/components/table_component.ex

Lines 284 to 288 in fe86d97

    
           def handle_event("select_filter", %{"filter" => filter}, socket) do 
        
             table_params = %{socket.assigns.table_params | filter: filter} 
        
             to = PageBuilder.live_dashboard_path(socket, socket.assigns.page, table_params) 
        
             {:noreply, push_patch(socket, to: to)} 
        
           end

By setting/updating @active_filter. This will have an effect in

phoenix_live_dashboard/lib/phoenix/live_dashboard/components/table_component.ex

Line 214 in fe86d97

<%= options_for_select(@filter_list, @active_filter) %>

This will usually happen just one time upon initialization of the table component:

phoenix_live_dashboard/lib/phoenix/live_dashboard/components/table_component.ex

Line 112 in fe86d97

|> assign(:active_filter, active_filter)}

, and from there on the value of the filter field would be controlled by the UI, unless for some reason (hot-swapping? or something else depending on the implementation of the process filter?) the value of @active_filter changes. If that's the case, the value of filter will be reset to the value of @active_filter again.
Edited: forgot to mention the case of switching between cluster nodes - this could also change @active_filter.

So the filter behaves differently from other table parameters in that it could be changed both by the backend and UI. Hence we have @active_filter as a bridge between the backend and UI.

Otherwise, the filter field is just another table parameter, so it needs to be handled in the same manner as other fields (i.e., limit, search, 'hint`...).

Please let me know if you see any problems with this description or the way to improve the code.

bokner · 2023-04-25T01:51:14Z

#411 (comment)
This should be fixed too.

Okay, maybe you could help me to fix it.

https://github.com/phoenixframework/phoenix_live_dashboard/pull/411/files/d146580eb01bee2bd1ce6c8b5ad7659d298d7aeb#diff-1d9200c19d147e3e691759ff70e93ff5287aa2cc8b475ee88ebfa04383acf159R96

This line is here because I wasn't able to make the condition in the form

phoenix_live_dashboard/lib/phoenix/live_dashboard/components/table_component.ex

Line 208 in fe86d97

    
                 <form :if={@active_filter} phx-change="select_filter" phx-target={@myself} class="form-inline">

to work without assigning active_filter. I'd expect :if not to fail if there is no assignment, but apparently, it's not the case. Please let me know if you see a way around it :-)

alexcastano · 2023-04-26T20:37:03Z

I don't think it is a problem to have always the "All" option. AFAIK the :observer works in a similar way.

It is a problem for us, and that's also the reason why we avoid using observer :-)

Edit:

Because we have a limit to don't showing every process it should be fine.

The issue is not so much with how many rows are shown, but rather with server-side polling a potentially huge number of processes and retrieving process info for each of them.

Fair enough. If you find this problem it makes sense to avoid it.

Sorry, but I don't get it. I don't see any @filter variable used in the template, either socket.assigns.filter in the rest of the code. If this is true, it doesn't make sense to have filter in the specs, in the Map.put_new and the attr.

You are right, there is none :-) I will try to convey my understanding of the workflow. I have set up a small application for the test, and the description of the workflow mostly comes from debugging it.
* `filter` is the name of the form field
  https://github.com/phoenixframework/phoenix_live_dashboard/blob/fe86d97d99c4025133240dea4d3b576872984183/lib/phoenix/live_dashboard/components/table_component.ex#L213

Yeah, this is true, but we can use any name in fact.

* The value of this field could be set in 2 ways
  
  * By the user selecting the value from the dropdown. This is handled by https://github.com/phoenixframework/phoenix_live_dashboard/blob/fe86d97d99c4025133240dea4d3b576872984183/lib/phoenix/live_dashboard/components/table_component.ex#L284-L288

This is not true. We are only changing the assigns.table_params.filter that is not the same as assigns.active_filter. This is a problem, and the source of our confusion: we are using two different variables to save the same value :)

Later, when we call fetch_rows it returns the 5-element tuple, it will return assigns.table_params.value to bet set to assigns.active_filter.

We use @limit or @search to say if the table has these features, if it can be limited, or if the user can search on it. And the current value of these inputs are stored in @table_params.limit and @table_params.search.

We cannot do exactly the same for filter because it is row_fetcher function that decides if the filter input exists or not depending on the returned value. So, I think we don't need an @active_filter attribute or not. Sorry for the confusion.

We should update @table_params.filter after calling row_fetcher. We need a @filter_list though, because we don't want to add it to @table_params. @filter_list is only set after calling row_fetcher, it cannot be set by the update/1 callback.

We should show the filter input only if @filter_list is not nil (or empty list I guess...) instead of using @active_filter. And then, for the value of the input, we should use @table_params.filter.

I think this way we fix all the issues related to this. I hope I explained better this time :)

lib/phoenix/live_dashboard/components/table_component.ex

lib/phoenix/live_dashboard/page_builder.ex

test/phoenix/live_dashboard/pages/applications_page_test.exs

bokner · 2023-04-27T13:45:47Z

We should show the filter input only if @filter_list is not nil (or empty list I guess...) instead of using @active_filter. And then, for the value of the input, we should use @table_params.filter.

Great, that makes sense, thank you @alexcastano for the explanation and patience :-)
I'll go ahead and make changes. Also, will try to rework fetch_rows a bit as per your suggestion.

Co-authored-by: Alex Castaño <alexcastanodev@gmail.com>

…e_dashboard into filtered_process_list * 'filtered_process_list' of github.com:bokner/phoenix_live_dashboard: Simplify the base implementation of default_filter/0

bokner · 2023-04-30T14:58:59Z

@alexcastano @josevalim I think I covered most of your suggestions. I also ran some manual tests, and it looks functional.
Please let me know how it looks to you when you get a chance :-)

alexcastano

Amazing work! It looks good to me :)

bokner added 7 commits February 19, 2023 11:36

Enable filtering of process PIDs

ff627d2

Fix process count

63bc1bf

Use the process naming if it's created by process filter

3e92e97

Customize process_info page

9f0c928

Let the callback decide what to do with the filter name

a8fb339

Behaviour for process filter

6c97999

Merge branch 'main' into filtered_process_list

e628820

Fixes

94265e3

Refactoring

a13b1a9

bokner force-pushed the filtered_process_list branch from d5021bd to a13b1a9 Compare March 3, 2023 17:05

bokner added 3 commits March 3, 2023 13:24

process_filter -> page_filter

ed5d42f

Refactoring, fixes

d1e4834

The content returned by the info_content/2 can be either of Phoenix.L…

9db7de8

…iveView.Rendered, list or nil

alexcastano reviewed Mar 7, 2023

View reviewed changes

bokner and others added 4 commits March 7, 2023 09:52

Update lib/phoenix/live_dashboard/page_filter.ex

1be224a

Co-authored-by: Alex Castaño <alexcastanodev@gmail.com>

Update lib/phoenix/live_dashboard/components/table_component.ex

fe0f424

Co-authored-by: Alex Castaño <alexcastanodev@gmail.com>

Update lib/phoenix/live_dashboard/pages/processes_page.ex

2c3a7be

Co-authored-by: Alex Castaño <alexcastanodev@gmail.com>

Update lib/phoenix/live_dashboard/components/table_component.ex

2b1f7c4

Co-authored-by: Alex Castaño <alexcastanodev@gmail.com>

alexcastano reviewed Apr 22, 2023

View reviewed changes

Have process filter tests for SystemInfo to run with async: false

e6d9ec1

alexcastano reviewed Apr 26, 2023

View reviewed changes

Unit tests for the filter in the process page

235a6d5

do not use active_filter; instead, update filter in fetch_rows

e056334

bokner force-pushed the filtered_process_list branch from 45e5abf to e056334 Compare April 28, 2023 14:21

Refactoring, fixes

408a28b

bokner force-pushed the filtered_process_list branch from 80499ee to 408a28b Compare April 29, 2023 17:19

bokner and others added 10 commits April 29, 2023 13:51

Simplify the base implementation of default_filter/0

a3b5137

Co-authored-by: Alex Castaño <alexcastanodev@gmail.com>

More refactoring

2076105

Merge branch 'filtered_process_list' of github.com:bokner/phoenix_liv…

d6e566b

…e_dashboard into filtered_process_list * 'filtered_process_list' of github.com:bokner/phoenix_live_dashboard: Simplify the base implementation of default_filter/0

Use default filter if the filter is not listed

ce63526

Minor

93d1fb5

Do not use hardcoded filter values in test cases

e988f05

Update docs for row_fetcher

a0ce9f8

Fix the invalid filter value handling

f826536

Move filter param down the list of params

fa3f109

Adjust filter_list to nil, if it's empty

7fe72ca

alexcastano approved these changes May 1, 2023

View reviewed changes

Merge branch 'phoenixframework:main' into filtered_process_list

e5c4b6f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filtered process list #411

Filtered process list #411

bokner commented Mar 2, 2023

josevalim commented Mar 2, 2023

bokner commented Mar 2, 2023

josevalim commented Mar 2, 2023

bokner commented Mar 2, 2023 •

edited

josevalim commented Mar 2, 2023

bokner commented Mar 2, 2023

josevalim commented Mar 2, 2023

josevalim commented Mar 2, 2023

bokner commented Mar 6, 2023

josevalim commented Mar 6, 2023

josevalim commented Mar 6, 2023

alexcastano left a comment

bokner commented Mar 7, 2023

alexcastano left a comment

bokner commented Apr 24, 2023

bokner commented Apr 24, 2023 •

edited

alexcastano commented Apr 24, 2023

alexcastano commented Apr 24, 2023

bokner commented Apr 24, 2023 •

edited

bokner commented Apr 24, 2023 •

edited

bokner commented Apr 25, 2023

alexcastano commented Apr 26, 2023

bokner commented Apr 27, 2023

bokner commented Apr 30, 2023

alexcastano left a comment

Filtered process list #411

Are you sure you want to change the base?

Filtered process list #411

Conversation

bokner commented Mar 2, 2023

josevalim commented Mar 2, 2023

bokner commented Mar 2, 2023

josevalim commented Mar 2, 2023

bokner commented Mar 2, 2023 • edited

josevalim commented Mar 2, 2023

bokner commented Mar 2, 2023

josevalim commented Mar 2, 2023

josevalim commented Mar 2, 2023

bokner commented Mar 6, 2023

josevalim commented Mar 6, 2023

josevalim commented Mar 6, 2023

alexcastano left a comment

Choose a reason for hiding this comment

bokner commented Mar 7, 2023

alexcastano left a comment

Choose a reason for hiding this comment

bokner commented Apr 24, 2023

bokner commented Apr 24, 2023 • edited

alexcastano commented Apr 24, 2023

alexcastano commented Apr 24, 2023

bokner commented Apr 24, 2023 • edited

bokner commented Apr 24, 2023 • edited

bokner commented Apr 25, 2023

alexcastano commented Apr 26, 2023

bokner commented Apr 27, 2023

bokner commented Apr 30, 2023

alexcastano left a comment

Choose a reason for hiding this comment

bokner commented Mar 2, 2023 •

edited

bokner commented Apr 24, 2023 •

edited

bokner commented Apr 24, 2023 •

edited

bokner commented Apr 24, 2023 •

edited