feat: metrics #629

anunaym14 · 2025-04-30T09:43:53Z

No description provided.

ilo-nanpa · 2025-04-30T09:43:58Z

it seems like you haven't added any nanpa changeset files to this PR.

if this pull request includes changes to code, make sure to add a changeset, by writing a file to .nanpa/<unique-name>.kdl:

minor type="added" "Introduce frobnication algorithm"

refer to the manpage for more information.

theomonnom · 2025-05-02T12:55:07Z

livekit-ffi/protocol/metrics.proto

Let's not copy this file, it is going to be very hard to keep it up to date since it's a copy (we will also have to release a new FFI versions each time we add a metric).

Instead, let's build the livekit metrics protocol on the Python side and serialize it before sending it to the FFI. The FFI is just going to be a passthrough for sending it over the DC.

You can take this example on how you can send raw bytes from Python:

rust-sdks/livekit-ffi/protocol/room.proto

Line 100 in 0773bce

required uint64 data_ptr = 2;

On Python, it can simply be generated for https://github.com/livekit/python-sdks/tree/main/livekit-protocol
On Node, we can reuse: https://github.com/livekit/protocol/tree/main/packages/javascript

Updated, please check.

theomonnom · 2025-05-02T12:56:06Z

livekit-ffi/protocol/ffi.proto

+  required uint64 local_participant_handle = 1;
+  repeated string str_data = 2;
+  repeated TimeSeriesMetric time_series = 3;
+  repeated EventMetric events = 4;
+  optional uint64 async_id = 5;


This will then become

Suggested change

required uint64 local_participant_handle = 1;

repeated string str_data = 2;

repeated TimeSeriesMetric time_series = 3;

repeated EventMetric events = 4;

optional uint64 async_id = 5;

required uint64 local_participant_handle = 1;

required uint64 data_ptr = 2;

required uint64 data_len = 3;

optional uint64 async_id = 4;

theomonnom · 2025-05-02T12:56:46Z

livekit/src/room/metrics/mod.rs

+                ..Default::default()
+            };
+
+            let _ = rtc_engine.publish_data(data_packet, DataPacketKind::Reliable).await;


Don't ignore errors, this function should return a Result

Handled, please check again

theomonnom · 2025-05-02T12:57:24Z

livekit/src/room/metrics/mod.rs

+    ) {
+        cancellation_token
+            .run_until_cancelled(async move {
+                std::thread::sleep(Duration::from_secs(1));


This is blocking the event loop, use a tokio sleep instead or an async interval instead

theomonnom · 2025-05-02T12:58:45Z

livekit/src/room/mod.rs

 pub struct Room {
    inner: Arc<RoomSession>,
+    cancellation_token: tokio_util::sync::CancellationToken


I think we can avoid the cancellation token. This seems unnecessary.

We can just keep track of the handle returned by tokio::spawn and await it on close.

To "close/cancel" the task, you can use a tokio::select pattern

Made it better, removed cancellation token, please have another look.

theomonnom · 2025-05-02T13:00:57Z

livekit/src/room/metrics/mod.rs

+        &self,
+        room: &Room,
+        rtc_engine: &RtcEngine,
+        cancellation_token: CancellationToken,


The cancellation can be done beforehand directly in the task inside Room using tokio::select!

This function should be cancel safe:
https://docs.rs/tokio/latest/tokio/macro.select.html#cancellation-safety

theomonnom · 2025-05-02T13:03:03Z

livekit/src/room/metrics/mod.rs

+            room.local_participant().identity().into(),
+        );
+
+        let _ = self.send_metrics(rtc_engine, strings, subscriber_ts_metrics).await;


Same here, let's return a Result

theomonnom · 2025-05-02T13:04:32Z

livekit/src/room/metrics/mod.rs

+    fn create_time_series(
+        &self,
+        label: MetricLabel,
+        strings: &mut Vec<String>,


Any reason why this need to be a mutable reference? It seems like the "caller" doesn't expect it? So maybe it's fine to clone inside this method and keep the modifications in this scope

Yes actually:

Strings are modified here:

rust-sdks/livekit/src/room/metrics/mod.rs

Line 54 in fd33fe8

strings.push(string);

but are created here:

rust-sdks/livekit/src/room/metrics/mod.rs

Line 230 in fd33fe8

let mut strings = Vec::new();

theomonnom · 2025-05-02T13:05:07Z

livekit/src/room/metrics/mod.rs

+            room.local_participant().identity().into(),
+        );
+
+        let _ = self.send_metrics(rtc_engine, strings, publisher_ts_metrics).await;


Same here, let's return a Result

theomonnom · 2025-05-02T13:08:50Z

livekit-ffi/generate_proto.sh

@@ -20,6 +20,8 @@ OUT_RUST=src
 protoc \
    -I=$PROTOCOL \
    --prost_out=$OUT_RUST \
+    --prost_opt=compile_well_known_types \
+    --prost_opt=extern_path=.google.protobuf=::pbjson_types \


Do we need json types? Seems like we never serialize to json?

Not anymore, livekit-protocol was converting .google.protobuf types to pbjson_types but livekit-ffi was not doing so, it was messing up timestamps in metrics.proto. Since we have removed metrics proto from ffi, we don't need this anymore.

anunaym14 added 2 commits April 28, 2025 17:22

feat(metrics): initial impl

a7612c4

try: ffi for metrics

2d21bfe

anunaym14 requested a review from theomonnom April 30, 2025 09:43

anunaym14 changed the title ~~Feat/metrics~~ feat: metrics Apr 30, 2025

anunaym14 force-pushed the feat/metrics branch from 075dd5a to aac4687 Compare April 30, 2025 15:10

fix: ffi build

6491025

anunaym14 force-pushed the feat/metrics branch from aac4687 to 6491025 Compare April 30, 2025 15:18

anunaym14 added 2 commits April 30, 2025 21:58

try: fix ios missing symbols

7dded2d

try: android: fix missing bits/libc-header-start.h

5b66474

anunaym14 force-pushed the feat/metrics branch from 61e13d9 to 5b66474 Compare April 30, 2025 17:55

anunaym14 and others added 4 commits May 1, 2025 00:04

try2

4e93e53

Update builds.yml

cd8c637

Update builds.yml

8a4a5b1

Update builds.yml

2cbf3d0

theomonnom reviewed May 2, 2025

View reviewed changes

pr comments

fd33fe8

anunaym14 force-pushed the feat/metrics branch from 0e93d36 to fd33fe8 Compare May 11, 2025 16:36

anunaym14 marked this pull request as ready for review May 12, 2025 08:17

chore: fmt

c6c5b17

feat: metrics #629

Are you sure you want to change the base?

feat: metrics #629

Uh oh!

Conversation

anunaym14 commented Apr 30, 2025

Uh oh!

ilo-nanpa bot commented Apr 30, 2025

Uh oh!

theomonnom May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anunaym14 May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theomonnom May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theomonnom May 2, 2025 •

edited

Loading

anunaym14 May 11, 2025 •

edited

Loading

theomonnom May 2, 2025 •

edited

Loading