From 56ebe568b253abf9f6947a0b4fdb9a985e2de89d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Feliks=20Pobiedzi=C5=84ski?= <38541925+FelonEkonom@users.noreply.github.com> Date: Wed, 4 Oct 2023 15:46:44 +0200 Subject: [PATCH] Sort component state fields in the error logs in the order from the most to the least important (#614) * defimpl Inspect for components states * Update lib/membrane/core/inspect.ex Co-authored-by: Mateusz Front * Update lib/membrane/core/inspect.ex Co-authored-by: Mateusz Front * Update lib/membrane/core/inspect.ex Co-authored-by: Mateusz Front * 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 5b1e1cbbd4fa8c05a4c77e47fb64c6c3833e1fbf. * Pull docker membrane latest before performance tests * Impelement reviewer suggestion * Add function doc * Update changelog --------- Co-authored-by: Mateusz Front --- .circleci/config.yml | 1 + CHANGELOG.md | 1 + lib/membrane/core/bin/pad_controller.ex | 7 ++- lib/membrane/core/bin/state.ex | 52 ++++++++++++------- lib/membrane/core/child/pad_spec_handler.ex | 3 +- lib/membrane/core/element/pad_controller.ex | 10 +++- lib/membrane/core/element/state.ex | 37 ++++++++----- lib/membrane/core/inspect.ex | 45 ++++++++++++++++ lib/membrane/core/pipeline/state.ex | 48 ++++++++++------- .../core/element/pad_controller_test.exs | 4 +- 10 files changed, 150 insertions(+), 58 deletions(-) create mode 100644 lib/membrane/core/inspect.ex diff --git a/.circleci/config.yml b/.circleci/config.yml index 26b93290c..fedce8b42 100644 --- 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 index 0b0bd70b2..7aae87a0e 100644 --- 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 index 39dc10a75..af6c448d7 100644 --- 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 index 982354fe6..24404c709 100644 --- 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 index e4921055e..70e53d075 100644 --- 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 index bcbf904d3..6b4d73ff9 100644 --- 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 index 437990430..141b53afa 100644 --- 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 new file mode 100644 index 000000000..39c68baf6 --- /dev/null +++ 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 index e845decc1..7caf0c064 100644 --- 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 index 9a25d880c..ad17195e1 100644 --- 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