Dashboard #571

mat-hek · 2023-06-27T13:48:24Z

Adds functionality needed for https://github.com/membraneframework/membrane_kino_dashboard

TODO:

add more metrics
- atomic demand
- auto demand
- input buffer

lib/membrane/core/observer.ex

test/membrane/core/element_test.exs

lib/membrane/core/observer.ex

FelonEkonom · 2023-06-28T14:16:22Z

lib/membrane/core/observer.ex

+  # observer if enabled by setting `unsafely_name_processes_for_observer: :components`
+  # in config.exs.
+  defp setup_process_local_observability(config, opts) do
+    utility_name = Map.get(opts, :utility_name)


Suggested change

utility_name = Map.get(opts, :utility_name)

utility_name = Map.get(opts, :utility_name, "")

FelonEkonom · 2023-06-28T15:06:55Z

lib/membrane/core/observer.ex

+
+    removed_links =
+      Enum.map(removed_links, fn link ->
+        link |> Map.take([:from, :to, :output, :input]) |> Map.put(:entity, :link)


but link.entity == :link because of line 429

FelonEkonom · 2023-06-28T15:12:23Z

lib/membrane/core/observer.ex

+
+  @impl true
+  def handle_info({:graph, graph_update}, state) do
+    {action, graph_entries, graph_updates, state} = handle_graph_update(graph_update, state)


Instead of updating value of state.graph sometimes in handle_graph_update and sometimes in handle_info, do it in only one place (IMO handle_graph_update is a good idea)

FelonEkonom · 2023-06-28T15:27:05Z

lib/membrane/core/observer.ex

+            {unquote(metric), unquote(opts)[:component_path] || ComponentPath.get(),
+             unquote(opts)[:pad]}
+
+          [{_key, value} | _default] = :ets.lookup(ets, key) ++ [{nil, unquote(init)}]


Getting metric value from ETS is slow, since we start ETS only with write_concurrency: true flag. Getting last value of metric from process dictionary would be better

lib/membrane/core/observer.ex

FelonEkonom · 2023-06-28T15:34:09Z

lib/membrane/core/observer.ex

+    register_name_for_observer(
+      :"##{unique_prefix}#{name_str}#{component_type_suffix}#{utility_name}"
+    )


IMO creating a new atom each time we spawn a component, is not a good idea. Where do we use this atom again?

Even if in some cases it is necessary, let's create it only when it will be used by register_name_for_observer/1

varsill · 2023-07-10T08:52:44Z

lib/membrane/core/bin.ex

@@ -92,7 +92,7 @@ defmodule Membrane.Core.Bin do
      log_metadata: options.log_metadata
    }

-    Membrane.Core.Observability.setup(observability_config)
+    Membrane.Core.Stalker.register_component(options.stalker, observability_config)


The name has exceeded all my expectations xD

I'm still open to better propositions :D

varsill · 2023-07-10T09:10:25Z

lib/membrane/core/child/pad_model.ex

+          total_buffers_metric: :atomics.atomics_ref(),
+          demand_metric: :atomics.atomics_ref()


wouldn't it be better to have a "single reference" to some "metrics struct" that would encapsulate these two references?

lib/membrane/core/element/input_queue.ex

varsill · 2023-07-11T07:58:00Z

lib/membrane/core/element/input_queue.ex

@@ -46,7 +47,8 @@ defmodule Membrane.Core.Element.InputQueue do
    :atomic_demand,
    :inbound_metric,
    :outbound_metric,
-    :linked_output_ref
+    :pad_ref,
+    :size_metric


Now we have inbound_metric and outbound_metric which describe the units used by particular pads, and size_metric which is used by our metrics mechanism.. Even though they are not connected, they are named similarly. I believe we need to think about some better namespacing in that case.

lib/membrane/core/stalker.ex

lib/membrane/core/element/buffer_controller.ex

lib/membrane/core/element/pad_controller.ex

lib/membrane/core/stalker.ex

varsill

I have left some two small things to consider, but generally: LGTM 🥇

varsill · 2023-07-14T07:53:35Z

lib/membrane/core/child/pad_model.ex

@@ -44,8 +44,7 @@ defmodule Membrane.Core.Child.PadModel do
          auto_demand_size: pos_integer() | nil,
          associated_pads: [Pad.ref()] | nil,
          sticky_events: [Membrane.Event.t()],
-          total_buffers_metric: :atomics.atomics_ref(),
-          demand_metric: :atomics.atomics_ref()
+          metrics: %{atom => :atomics.atomics_ref()}


wouldn't it be better to have stalker_metrics here?

varsill · 2023-07-14T07:54:31Z

lib/membrane/core/element/input_queue.ex

@@ -37,7 +37,8 @@ defmodule Membrane.Core.Element.InputQueue do
          inbound_metric: module(),
          outbound_metric: module(),
          pad_ref: Pad.ref(),
-          atomic_demand: AtomicDemand.t()
+          atomic_demand: AtomicDemand.t(),
+          stalker_metrics: map()


[NIT] How about:

Suggested change

stalker_metrics: map()

stalker_metrics: %{atom() => any()}

```?

Co-authored-by: Łukasz Kita <[email protected]>

mat-hek requested review from varsill and FelonEkonom June 27, 2023 13:48

mat-hek added the no-changelog This label has to be added if changes from the PR are not meant to be placed in the CHANGELOG.md label Jun 27, 2023

varsill requested changes Jun 29, 2023

View reviewed changes

FelonEkonom requested changes Jun 29, 2023

View reviewed changes

mat-hek force-pushed the dashboard branch 14 times, most recently from e8effb0 to e79772f Compare July 7, 2023 10:35

mat-hek requested review from varsill and FelonEkonom July 7, 2023 13:07

varsill reviewed Jul 11, 2023

View reviewed changes

FelonEkonom requested changes Jul 11, 2023

View reviewed changes

lib/membrane/core/element/buffer_controller.ex Outdated Show resolved Hide resolved

lib/membrane/core/element/pad_controller.ex Show resolved Hide resolved

lib/membrane/core/stalker.ex Show resolved Hide resolved

mat-hek requested review from varsill and FelonEkonom July 12, 2023 14:20

FelonEkonom approved these changes Jul 13, 2023

View reviewed changes

varsill approved these changes Jul 14, 2023

View reviewed changes

mat-hek added 3 commits July 14, 2023 10:51

add observability for dashboard

59b18f4

fix tests

de31fcb

observer metric id -> pad

7981e0f

mat-hek and others added 14 commits July 14, 2023 10:51

observer: cleanup metrics on unlink/death

601e3d0

add observer tests

b650e10

Update lib/membrane/core/observer.ex

12d4f1a

Co-authored-by: Łukasz Kita <[email protected]>

WIP metric functions

e2da30a

atomic demand metric

832a9d0

demand metric, input queue size metric

77050fc

fix tests

de2328f

changes for CR

f7fae5c

rename observer to stalker

95b1d24

refactor observability setup

1f1657f

remove kino from deps

a284534

fix stalker time rounding

b6b3f92

fixes for CR

569e19d

pad_data.metrics -> stalker_metrics

085e418

mat-hek force-pushed the dashboard branch from c448881 to 085e418 Compare July 14, 2023 08:52

mat-hek merged commit 1f3c9bd into master Jul 14, 2023
1 check passed

mat-hek deleted the dashboard branch July 14, 2023 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard #571

Dashboard #571

mat-hek commented Jun 27, 2023 •

edited

Loading

FelonEkonom Jun 28, 2023

FelonEkonom Jun 28, 2023

FelonEkonom Jun 28, 2023

FelonEkonom Jun 28, 2023

FelonEkonom Jun 28, 2023

FelonEkonom Jun 29, 2023

varsill Jul 10, 2023

mat-hek Jul 11, 2023

varsill Jul 10, 2023

varsill Jul 11, 2023

varsill left a comment

varsill Jul 14, 2023

varsill Jul 14, 2023

	utility_name = Map.get(opts, :utility_name)
	utility_name = Map.get(opts, :utility_name, "")

		total_buffers_metric: :atomics.atomics_ref(),
		demand_metric: :atomics.atomics_ref()

	stalker_metrics: map()
	stalker_metrics: %{atom() => any()}
	```?

Dashboard #571

Dashboard #571

Conversation

mat-hek commented Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varsill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-hek commented Jun 27, 2023 •

edited

Loading