Sort component state fields in the error logs in the order from the m…

…ost to the least important (#614) * defimpl Inspect for components states * Update lib/membrane/core/inspect.ex Co-authored-by: Mateusz Front <[email protected]> * Update lib/membrane/core/inspect.ex Co-authored-by: Mateusz Front <[email protected]> * Update lib/membrane/core/inspect.ex Co-authored-by: Mateusz Front <[email protected]> * Implement suggestion from CR, add new field to the state * Fix compilation error * Specify docker_membrane version to latest * Revert "Fix compilation error" This reverts commit 5b1e1cb. * Pull docker membrane latest before performance tests * Impelement reviewer suggestion * Add function doc * Update changelog --------- Co-authored-by: Mateusz Front <[email protected]>
membraneframework · Oct 4, 2023 · 56ebe56 · 56ebe56
1 parent 4d4283d
commit 56ebe56
Show file tree

Hide file tree

Showing 10 changed files with 150 additions and 58 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -154,6 +154,7 @@ jobs:
           at: .
       - run: cp -r benchmark/ ~/benchmark_backup/
       - run: cp mix.exs ~/benchmark_backup/
+      - run: docker pull membraneframeworklabs/docker_membrane
       - run: docker run -e MIX_ENV=benchmark -v ./:/root/app -v ~/results:/root/results -w /root/app membraneframeworklabs/docker_membrane mix do deps.get, deps.compile --force --all, run benchmark/run.exs /root/results/feature_branch_results
       - run: git checkout -f master
       - run: cp  ~/benchmark_backup/mix.exs ~/app

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,6 +26,7 @@
  * Add child exit reason to the supervisor exit reason. [#595](https://github.com/membraneframework/membrane_core/pull/595)
  * Remove default implementation of `start_/2`, `start_link/2` and `terminate/2` in modules using `Membrane.Pipeline`. [#598](https://github.com/membraneframework/membrane_core/pull/598) 
  * Remove callback _Membrane.Element.WithInputPads.handle_buffers_batch/4_. [#601](https://github.com/membraneframework/membrane_core/pull/601)
+ * Sort component state fields in the error logs in the order from the most to the least important. [#614](https://github.com/membraneframework/membrane_core/pull/614)
 
 ## 0.11.0
  * Separate element_name and pad arguments in handle_element_{start, end}_of_stream signature [#219](https://github.com/membraneframework/membrane_core/issues/219)

diff --git a/lib/membrane/core/bin/pad_controller.ex b/lib/membrane/core/bin/pad_controller.ex
@@ -51,6 +51,7 @@ defmodule Membrane.Core.Bin.PadController do
       case PadModel.get_data(state, pad_ref) do
         {:error, :unknown_pad} ->
           init_pad_data(pad_ref, pad_info, state)
+          |> Map.update!(:pad_refs, &[pad_ref | &1])
 
         # This case is for pads that were instantiated before the external link request,
         # that is in the internal link request (see `handle_internal_link_request/4`).
@@ -281,8 +282,10 @@ defmodule Membrane.Core.Bin.PadController do
   @spec handle_unlink(Pad.ref(), Core.Bin.State.t()) :: Core.Bin.State.t()
   def handle_unlink(pad_ref, state) do
     with {:ok, %{availability: :on_request}} <- PadModel.get_data(state, pad_ref) do
-      state = maybe_handle_pad_removed(pad_ref, state)
-      {pad_data, state} = PadModel.pop_data!(state, pad_ref)
+      {pad_data, state} =
+        maybe_handle_pad_removed(pad_ref, state)
+        |> Map.update!(:pad_refs, &List.delete(&1, pad_ref))
+        |> PadModel.pop_data!(pad_ref)
 
       if pad_data.endpoint do
         Message.send(pad_data.endpoint.pid, :handle_unlink, pad_data.endpoint.pad_ref)

diff --git a/lib/membrane/core/bin/state.ex b/lib/membrane/core/bin/state.ex
@@ -8,7 +8,7 @@ defmodule Membrane.Core.Bin.State do
   use Bunch
   use Bunch.Access
 
-  alias Membrane.{Child, Clock, Sync}
+  alias Membrane.{Child, Clock, Pad, Sync}
   alias Membrane.Core.Child.PadModel
   alias Membrane.Core.Parent.ChildLifeController
   alias Membrane.Core.Parent.{ChildrenModel, CrashGroup, Link}
@@ -20,6 +20,7 @@ defmodule Membrane.Core.Bin.State do
           children: ChildrenModel.children(),
           subprocess_supervisor: pid(),
           name: Membrane.Bin.name() | nil,
+          pad_refs: [Pad.ref()],
           pads_info: PadModel.pads_info() | nil,
           pads_data: PadModel.pads_data() | nil,
           parent_pid: pid,
@@ -49,23 +50,34 @@ defmodule Membrane.Core.Bin.State do
           stalker: Membrane.Core.Stalker.t()
         }
 
-  @enforce_keys [:module, :synchronization, :subprocess_supervisor, :resource_guard, :stalker]
-  defstruct @enforce_keys ++
-              [
-                internal_state: nil,
-                children: %{},
-                name: nil,
-                pads_info: nil,
-                pads_data: nil,
-                parent_pid: nil,
-                crash_groups: %{},
-                children_log_metadata: [],
-                links: %{},
-                pending_specs: %{},
-                playback: :stopped,
-                initialized?: false,
-                terminating?: false,
-                setup_incomplete?: false,
-                handling_action?: false
-              ]
+  # READ THIS BEFORE ADDING NEW FIELD!!!
+
+  # Fields of this structure will be inspected in the same order, in which they occur in the
+  # list passed to `defstruct`. Take a look at lib/membrane/core/inspect.ex to get more info.
+  # If you want to add a new field to the state, place it at the spot corresponding to its
+  # importance and possibly near other related fields. It is suggested, to keep `:pads_data`
+  # as the last item in the list, because sometimes it is so big, that everything after it
+  # might be truncated during the inspection.
+
+  defstruct module: nil,
+            name: nil,
+            parent_pid: nil,
+            playback: :stopped,
+            internal_state: nil,
+            pad_refs: [],
+            pads_info: nil,
+            children: %{},
+            links: %{},
+            crash_groups: %{},
+            pending_specs: %{},
+            synchronization: nil,
+            initialized?: false,
+            terminating?: false,
+            setup_incomplete?: false,
+            handling_action?: false,
+            stalker: nil,
+            resource_guard: nil,
+            subprocess_supervisor: nil,
+            children_log_metadata: [],
+            pads_data: nil
 end
diff --git a/lib/membrane/core/child/pad_spec_handler.ex b/lib/membrane/core/child/pad_spec_handler.ex
@@ -21,7 +21,8 @@ defmodule Membrane.Core.Child.PadSpecHandler do
       | pads_info:
           get_pads(state)
           |> Map.new(),
-        pads_data: %{}
+        pads_data: %{},
+        pad_refs: []
     }
   end
 

diff --git a/lib/membrane/core/element/pad_controller.ex b/lib/membrane/core/element/pad_controller.ex
@@ -228,7 +228,10 @@ defmodule Membrane.Core.Element.PadController do
       state = generate_eos_if_needed(pad_ref, state)
       state = maybe_handle_pad_removed(pad_ref, state)
       state = remove_pad_associations(pad_ref, state)
-      {pad_data, state} = PadModel.pop_data!(state, pad_ref)
+
+      {pad_data, state} =
+        Map.update!(state, :pad_refs, &List.delete(&1, pad_ref))
+        |> PadModel.pop_data!(pad_ref)
 
       with %{direction: :input, flow_control: :auto, other_effective_flow_control: :pull} <-
              pad_data do
@@ -314,7 +317,10 @@ defmodule Membrane.Core.Element.PadController do
       |> merge_pad_mode_data(endpoint.pad_props, other_pad_info, state)
       |> then(&struct!(Membrane.Element.PadData, &1))
 
-    state = put_in(state, [:pads_data, endpoint.pad_ref], pad_data)
+    state =
+      state
+      |> put_in([:pads_data, endpoint.pad_ref], pad_data)
+      |> Map.update!(:pad_refs, &[endpoint.pad_ref | &1])
 
     :ok =
       AtomicDemand.set_sender_status(

diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex
@@ -19,6 +19,7 @@ defmodule Membrane.Core.Element.State do
           type: Element.type(),
           name: Element.name(),
           internal_state: Element.state() | nil,
+          pad_refs: [Pad.ref()] | nil,
           pads_info: PadModel.pads_info() | nil,
           pads_data: PadModel.pads_data() | nil,
           parent_pid: pid,
@@ -45,29 +46,39 @@ defmodule Membrane.Core.Element.State do
           stalker: Membrane.Core.Stalker.t()
         }
 
+  # READ THIS BEFORE ADDING NEW FIELD!!!
+
+  # Fields of this structure will be inspected in the same order, in which they occur in the
+  # list passed to `defstruct`. Take a look at lib/membrane/core/inspect.ex to get more info.
+  # If you want to add a new field to the state, place it at the spot corresponding to its
+  # importance and possibly near other related fields. It is suggested, to keep `:pads_data`
+  # as the last item in the list, because sometimes it is so big, that everything after it
+  # might be truncated during the inspection.
+
   defstruct [
     :module,
-    :type,
     :name,
+    :parent_pid,
+    :playback,
+    :type,
     :internal_state,
+    :pad_refs,
     :pads_info,
-    :pads_data,
-    :parent_pid,
-    :supplying_demand?,
-    :delayed_demands,
-    :handle_demand_loop_counter,
     :synchronization,
-    :demand_size,
+    :delayed_demands,
+    :effective_flow_control,
     :initialized?,
-    :playback,
-    :playback_queue,
-    :resource_guard,
-    :subprocess_supervisor,
     :terminating?,
     :setup_incomplete?,
-    :effective_flow_control,
+    :supplying_demand?,
     :handling_action?,
+    :stalker,
+    :resource_guard,
+    :subprocess_supervisor,
+    :handle_demand_loop_counter,
+    :demand_size,
     :pads_to_snapshot,
-    :stalker
+    :playback_queue,
+    :pads_data
   ]
 end
diff --git a/lib/membrane/core/inspect.ex b/lib/membrane/core/inspect.ex
@@ -0,0 +1,45 @@
+defmodule Membrane.Core.Inspect do
+  @moduledoc false
+
+  alias Inspect.Algebra
+  alias Membrane.Core.Component
+
+  @doc """
+  A function, that inspects passed state sorting its fields withing the order in which
+  they occur in the list passed to `defstruct`.
+  """
+  @spec inspect(Component.state(), Inspect.Opts.t()) :: Inspect.Algebra.t()
+  def inspect(%state_module{} = state, opts) do
+    ordered_fields =
+      state_module.__info__(:struct)
+      |> Enum.map(& &1.field)
+
+    field_to_doc_fun =
+      fn field, opts ->
+        value_doc =
+          Map.fetch!(state, field)
+          |> Algebra.to_doc(opts)
+
+        Algebra.concat("#{Atom.to_string(field)}: ", value_doc)
+      end
+
+    Algebra.container_doc(
+      "%#{Kernel.inspect(state_module)}{",
+      ordered_fields,
+      "}",
+      opts,
+      field_to_doc_fun,
+      break: :strict
+    )
+  end
+end
+
+[Pipeline, Bin, Element]
+|> Enum.map(fn component ->
+  state_module = Module.concat([Membrane.Core, component, State])
+
+  defimpl Inspect, for: state_module do
+    @spec inspect(unquote(state_module).t(), Inspect.Opts.t()) :: Inspect.Algebra.t()
+    defdelegate inspect(state, opts), to: Membrane.Core.Inspect
+  end
+end)
diff --git a/lib/membrane/core/pipeline/state.ex b/lib/membrane/core/pipeline/state.ex
@@ -9,15 +9,17 @@ defmodule Membrane.Core.Pipeline.State do
   use Bunch.Access
 
   alias Membrane.Child
-  alias Membrane.Core.Parent.{ChildrenModel, CrashGroup, Link}
+  alias Membrane.Core.Parent.{ChildLifeController, ChildrenModel, CrashGroup, Link}
   alias Membrane.Core.Timer
 
   @type t :: %__MODULE__{
-          internal_state: Membrane.Pipeline.state() | nil,
           module: module,
+          playback: Membrane.Playback.t(),
+          internal_state: Membrane.Pipeline.state() | nil,
           children: ChildrenModel.children(),
-          crash_groups: %{CrashGroup.name() => CrashGroup.t()},
           links: %{Link.id() => Link.t()},
+          crash_groups: %{CrashGroup.name() => CrashGroup.t()},
+          pending_specs: ChildLifeController.pending_specs(),
           synchronization: %{
             timers: %{Timer.id() => Timer.t()},
             clock_provider: %{
@@ -27,27 +29,35 @@ defmodule Membrane.Core.Pipeline.State do
             },
             clock_proxy: Membrane.Clock.t()
           },
-          playback: Membrane.Playback.t(),
           initialized?: boolean(),
           terminating?: boolean(),
           resource_guard: Membrane.ResourceGuard.t(),
           setup_incomplete?: boolean(),
           handling_action?: boolean(),
-          stalker: Membrane.Core.Stalker.t()
+          stalker: Membrane.Core.Stalker.t(),
+          subprocess_supervisor: pid()
         }
 
-  @enforce_keys [:module, :synchronization, :subprocess_supervisor, :resource_guard, :stalker]
-  defstruct @enforce_keys ++
-              [
-                internal_state: nil,
-                children: %{},
-                crash_groups: %{},
-                links: %{},
-                pending_specs: %{},
-                playback: :stopped,
-                initialized?: false,
-                terminating?: false,
-                setup_incomplete?: false,
-                handling_action?: false
-              ]
+  # READ THIS BEFORE ADDING NEW FIELD!!!
+
+  # Fields of this structure will be inspected in the same order, in which they occur in the
+  # list passed to `defstruct`. Take a look at lib/membrane/core/inspect.ex to get more info.
+  # If you want to add a new field to the state, place it at the spot corresponding to its
+  # importance and possibly near other related fields.
+
+  defstruct module: nil,
+            playback: :stopped,
+            internal_state: nil,
+            children: %{},
+            links: %{},
+            crash_groups: %{},
+            pending_specs: %{},
+            synchronization: nil,
+            initialized?: false,
+            terminating?: false,
+            setup_incomplete?: false,
+            handling_action?: false,
+            stalker: nil,
+            resource_guard: nil,
+            subprocess_supervisor: nil
 end
diff --git a/test/membrane/core/element/pad_controller_test.exs b/test/membrane/core/element/pad_controller_test.exs
@@ -61,7 +61,9 @@ defmodule Membrane.Core.Element.PadControllerTest do
                  state
                )
 
-      assert %{new_state | pads_data: nil} == %{state | pads_data: nil}
+      assert Map.drop(new_state, [:pads_data, :pad_refs]) ==
+               Map.drop(state, [:pads_data, :pad_refs])
+
       assert PadModel.assert_instance(new_state, :input) == :ok
     end