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

Fce 817 rtp metrics #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fce 817 rtp metrics #23

wants to merge 1 commit into from

Conversation

roznawsk
Copy link
Collaborator

No description provided.

Copy link

linear bot commented Nov 27, 2024

@roznawsk roznawsk force-pushed the FCE-817-rtp-metrics branch from f73c335 to da61af6 Compare November 27, 2024 18:27
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 30 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...c/lib/ex_webrtc/peer_connection_handler/metrics.ex 8.69% 21 Missing ⚠️
ex_webrtc/lib/ex_webrtc/peer_connection_handler.ex 47.05% 9 Missing ⚠️
@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   57.26%   57.14%   -0.13%     
==========================================
  Files          57       60       +3     
  Lines        2607     2672      +65     
==========================================
+ Hits         1493     1527      +34     
- Misses       1114     1145      +31     
Files with missing lines Coverage Δ
ex_webrtc/lib/ex_webrtc/peer_connection_handler.ex 63.42% <47.05%> (-1.39%) ⬇️
...c/lib/ex_webrtc/peer_connection_handler/metrics.ex 8.69% <8.69%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 410d7de...ef99d6e. Read the comment docs.

@roznawsk roznawsk marked this pull request as ready for review November 28, 2024 11:15
@roznawsk roznawsk requested a review from Karolk99 November 28, 2024 11:15
@roznawsk roznawsk force-pushed the FCE-817-rtp-metrics branch 3 times, most recently from aaa2ebb to 04f2a04 Compare December 3, 2024 12:16
Copy link
Collaborator

@Karolk99 Karolk99 left a comment

Choose a reason for hiding this comment

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

Also integration tests are failing

Comment on lines +10 to +16
TestVideoroomWeb.Endpoint,
{Membrane.TelemetryMetrics.Reporter,
[
metrics: Membrane.RTC.Engine.Endpoint.ExWebRTC.Metrics.metrics(),
name: ExWebrtcMetricsReporter
]},
TestVideoroom.MetricsScraper
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the advantage of using Membrane.TelemetryMetrics instead of regular telemetry in case of ex_webrtc_endpoint ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The advantage of using Membrane.TelemetryMetrics over regular Telemetry.Metrics, especially in the context of ex_webrtc_endpoint, stems from the specific adaptations in Membrane.TelemetryMetrics that are tailored for multimedia applications. Felix mentioned that regular Telemetry.Metrics might not be sufficiently suited for multimedia use cases. Furthermore, Membrane.TelemetryMetrics includes certain optimizations or "hacks" that you would need to manually implement if you were using Telemetry.Metrics, enhancing efficiency and effectiveness for multimedia-specific metrics collection and monitoring in the Membrane framework.

Comment on lines 214 to 250
defp handle_entry(%{type: :inbound_rtp} = entry, telemetry_label) do
telemetry_label = telemetry_label ++ [track_id: entry.track_identifier]

TelemetryMetrics.execute(
@inbound_rtp_event,
Map.take(entry, [
:packets_received,
:bytes_received,
:nack_count,
:pli_count,
:markers_received,
:codec
]),
%{},
telemetry_label
)
end

defp handle_entry(%{type: :outbound_rtp} = entry, telemetry_label) do
telemetry_label = telemetry_label ++ [track_id: entry.track_identifier]

TelemetryMetrics.execute(
@outbound_rtp_event,
Map.take(entry, [
:packets_sent,
:bytes_sent,
:nack_count,
:pli_count,
:markers_sent,
:codec,
:retransmitted_packets_sent,
:retransmitted_bytes_sent
]),
%{},
telemetry_label
)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does it work in the case of simulcast?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We track each variant separately (the track_identifier has a variant in its name)

default: [],
description: "Label passed to Membrane.TelemetryMetrics functions"
],
get_stats_interval: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about config.exs instead of endpoint option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea

@roznawsk roznawsk requested a review from Karolk99 December 4, 2024 17:16
@roznawsk roznawsk force-pushed the FCE-817-rtp-metrics branch from db725ec to df46999 Compare December 6, 2024 16:20
@roznawsk roznawsk force-pushed the FCE-817-rtp-metrics branch from df46999 to ef99d6e Compare December 6, 2024 16:24
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