Save tracking info for rooms #1699

mjh1 · 2023-04-24T18:29:09Z

What does this pull request do? Explain your changes. (required)

I've created a database table to be able to keep track of rooms. I've implemented a webhook receiver so that we can store information like which participants have joined and what tracks they have published.

Specific updates (required)

How did you test each of these updates (required)

Modified existing unit test to cover new functionality.
Tested webhooks locally to observe the expected updated info in the room table.

Does this pull request close any open issues?

Screenshots (optional)

Checklist

I have read the CONTRIBUTING document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.

vercel · 2023-04-24T18:29:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
livepeer-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 26, 2023 2:26pm

packages/api/src/controllers/room.ts

codecov · 2023-04-24T18:40:40Z

Codecov Report

Merging #1699 (5ba89c8) into master (7341a28) will decrease coverage by 0.25826%.
The diff coverage is 31.66667%.

@@                 Coverage Diff                 @@
##              master       #1699         +/-   ##
===================================================
- Coverage   53.02876%   52.77050%   -0.25826%     
===================================================
  Files             74          75          +1     
  Lines           4903        4963         +60     
  Branches         976         987         +11     
===================================================
+ Hits            2600        2619         +19     
- Misses          1970        2007         +37     
- Partials         333         337          +4

Impacted Files	Coverage Δ
packages/api/src/parse-cli.ts	`34.04255% <ø> (ø)`
packages/api/src/store/table.ts	`71.68675% <ø> (ø)`
packages/api/src/controllers/room.ts	`29.31034% <29.31034%> (ø)`
packages/api/src/store/db.ts	`82.60870% <100.00000%> (+0.19112%)`	⬆️
packages/api/src/test-server.ts	`93.18182% <100.00000%> (+0.15856%)`	⬆️

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 cbdbc0d...5ba89c8. Read the comment docs.

Impacted Files	Coverage Δ
packages/api/src/parse-cli.ts	`34.04255% <ø> (ø)`
packages/api/src/store/table.ts	`71.68675% <ø> (ø)`
packages/api/src/controllers/room.ts	`29.31034% <29.31034%> (ø)`
packages/api/src/store/db.ts	`82.60870% <100.00000%> (+0.19112%)`	⬆️
packages/api/src/test-server.ts	`93.18182% <100.00000%> (+0.15856%)`	⬆️

victorges

LGTM

victorges · 2023-04-25T22:19:22Z

packages/api/src/controllers/room.ts

-  const room = await svc.createRoom(opts);
+    emptyTimeout: 15 * 60,
+    maxParticipants: 10,
+    metadata: `{"userId": "${req.user.id}"}`,


Nice to have the metadata! As a nit, you could write a syntax-safe JSON here as well:

Suggested change

metadata: `{"userId": "${req.user.id}"}`,

metadata: JSON.stringify({userId: req.user.id}),

victorges · 2023-04-25T22:23:47Z

packages/api/src/controllers/room.ts

+app.use("/webhook", express.raw({ type: "*/*" }));
+app.post("/webhook", async (req, res) => {


nit: I think this would have the same effect (unless you want the middleware on other methods not only POST)

Suggested change

app.use("/webhook", express.raw({ type: "*/*" }));

app.post("/webhook", async (req, res) => {

app.post("/webhook", express.raw({ type: "*/*" }), async (req, res) => {

victorges · 2023-04-25T22:24:46Z

packages/api/src/controllers/room.ts

+    req.config.livekitApiKey,
+    req.config.livekitSecret
+  );
+  console.log("livekit webhook auth", req.get("Authorization"));


Should probably remove/comment this log before deploy

victorges · 2023-04-25T22:26:06Z

packages/api/src/controllers/room.ts

+      );
+      const participants = await svc.listParticipants(roomId);
+
+      room = await db.room.get(roomId);


Maybe double check this room exists and return a 4xx error if it doesn't (or anything that LiveKit won't retry)

victorges · 2023-04-25T22:28:36Z

packages/api/src/schema/schema.yaml

+          description: Success
+          content:
+            application/json:
+              $ref: "#/components/schemas/create-room-response"


If it's only for $refing it here, I'd say we can just inline it (and remove the top-level component)

Suggested change

$ref: "#/components/schemas/create-room-response"

additionalProperties: false

properties:

id:

type: string

victorges · 2023-04-25T22:29:27Z

packages/api/src/schema/schema.yaml

+          unique: true
+          index: true


Actually the id field is the only exception! We don't need any index here.

Suggested change

unique: true

index: true

In the database, we have a separate column for the ID which is also the table primary key, and when querying by it we always check that table instead of the id in the JSON column. The index here would be redundant/unused.

victorges · 2023-04-25T22:31:17Z

packages/api/src/schema/schema.yaml

+          type: string
+          readOnly: true
+          example: 66E2161C-7670-4D05-B71D-DA2D6979556F
+          index: true


You need to add room to this list here: https://github.com/livepeer/livepeer-com/blob/a326c2f31bfd7638390ba703bcf7f1ca6a7cad1e/packages/api/src/store/table.ts#L352

Otherwise the indexes won't really be created. This is a tech debt we've been carrying since the time we had some issues with indexes on the stream table which is huge and gets a lot of updates.

victorges · 2023-04-25T22:32:51Z

packages/api/src/schema/schema.yaml

+        id:
+          type: string
+
+    room:


Would you mind adding documentation (description) to the user-facing fields in this object?

victorges · 2023-04-25T22:34:52Z

packages/api/src/controllers/room.ts

+  for (const key in room.participants) {
+    room.participants[key].tracksPublished = undefined;
+  }


Maybe consider hiding the entire participants field. Do users really need access to all that information? Might be easier to start by using it only internally for tracking/billing and consider exposing only parts of it that users may need.

I think it will be helpful to give them at least the IDs and names of the participants so that they would easily be able to see who has joined and remove people if needed (api coming in next PR). I will tidy up the remaining fields too in the next PR.

victorges · 2023-04-25T22:36:39Z

packages/api/src/controllers/room.ts

+      break;
+    case "room_started":
+    case "room_finished":
+      room = await db.room.get(event.room.name);


Same for checking if the room exists. Maybe we want this step outside the switch?

packages/api/src/controllers/room.ts

* Save tracking info for rooms * add response schemas * review comments

Save tracking info for rooms

17c0ea1

mjh1 requested a review from a team as a code owner April 24, 2023 18:29

mjh1 requested a review from victorges April 24, 2023 18:29

github-advanced-security bot found potential problems Apr 24, 2023

View reviewed changes

packages/api/src/controllers/room.ts Fixed Show fixed Hide fixed

mjh1 requested a review from gioelecerati April 25, 2023 09:09

add response schemas

48a5508

vercel bot deployed to Preview April 25, 2023 13:03 View deployment

victorges approved these changes Apr 25, 2023

View reviewed changes

vercel bot deployed to Preview April 26, 2023 13:56 View deployment

review comments

5ba89c8

mjh1 force-pushed the mh/livekit-track branch from d8bbcc7 to 5ba89c8 Compare April 26, 2023 14:24

vercel bot deployed to Preview April 26, 2023 14:26 View deployment

github-advanced-security bot found potential problems Apr 26, 2023

View reviewed changes

packages/api/src/controllers/room.ts Dismissed Show dismissed Hide dismissed

mjh1 merged commit cc7102b into master Apr 26, 2023
18 checks passed

mjh1 deleted the mh/livekit-track branch April 26, 2023 16:03

suhailkakar pushed a commit that referenced this pull request May 4, 2023

Save tracking info for rooms (#1699)

35f6126

* Save tracking info for rooms * add response schemas * review comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save tracking info for rooms #1699

Save tracking info for rooms #1699

mjh1 commented Apr 24, 2023 •

edited

vercel bot commented Apr 24, 2023 •

edited

codecov bot commented Apr 24, 2023 •

edited

victorges left a comment

victorges Apr 25, 2023

victorges Apr 25, 2023

victorges Apr 25, 2023

victorges Apr 25, 2023

victorges Apr 25, 2023

victorges Apr 25, 2023

victorges Apr 25, 2023

victorges Apr 25, 2023

victorges Apr 25, 2023

mjh1 Apr 26, 2023

victorges Apr 25, 2023

	metadata: `{"userId": "${req.user.id}"}`,
	metadata: JSON.stringify({userId: req.user.id}),

		app.use("/webhook", express.raw({ type: "/" }));
		app.post("/webhook", async (req, res) => {

	app.use("/webhook", express.raw({ type: "/" }));
	app.post("/webhook", async (req, res) => {
	app.post("/webhook", express.raw({ type: "/" }), async (req, res) => {

Save tracking info for rooms #1699

Save tracking info for rooms #1699

Conversation

mjh1 commented Apr 24, 2023 • edited

vercel bot commented Apr 24, 2023 • edited

codecov bot commented Apr 24, 2023 • edited

Codecov Report

victorges left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjh1 commented Apr 24, 2023 •

edited

vercel bot commented Apr 24, 2023 •

edited

codecov bot commented Apr 24, 2023 •

edited