Skip to content

allow flexible recording options#4758

Merged
theomonnom merged 24 commits intomainfrom
dz/recording-options
Feb 16, 2026
Merged

allow flexible recording options#4758
theomonnom merged 24 commits intomainfrom
dz/recording-options

Conversation

@davidzhao
Copy link
Member

@davidzhao davidzhao commented Feb 7, 2026

when starting a session, we now support record=RecordOptions() where audio, traces, logs, and transcript could be individually enabled or disabled.


Open with Devin

when starting a session, we now support record=RecordOptions() where
audio, traces, logs, and transcript could be individually enabled or
disabled.
@chenghao-mou chenghao-mou requested a review from a team February 7, 2026 20:29
chatgpt-codex-connector[bot]

This comment was marked as resolved.

Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

Open in Devin Review

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

davidzhao and others added 2 commits February 7, 2026 15:43
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Copy link
Member

@chenghao-mou chenghao-mou left a comment

Choose a reason for hiding this comment

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

One failing test, otherwise lgtm.

One thing I noticed is that we show "No observability data available" on the UI whenever traces aren't available (even when other types of data should be).

# wait for the idle processes to be warmed up (by the main task)
await self._idle_ready.wait()
# use a timeout so start() doesn't block forever if initialization fails
try:
Copy link
Member Author

Choose a reason for hiding this comment

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

@theomonnom confirming that this should not block .start if processes take longer than init timeout

Copy link
Member

Choose a reason for hiding this comment

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

If initialization fails, other processes will try to be spawned again. So it shouldn't block forever even without a timeout

Copy link
Member Author

Choose a reason for hiding this comment

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

without this, test_slow_initialization could not start correctly... because I think each new process is hitting the timeout, so it was never able to reach capacity. wdty the right behavior should be?

Copy link
Member

@theomonnom theomonnom Feb 10, 2026

Choose a reason for hiding this comment

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

shouldn't await _wait_for_elements(start_q, num_idle_processes) already wait for those processes tho?

Copy link
Member

Choose a reason for hiding this comment

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

But I think you're right, we shouldn't wait here, can you ignore it for this PR? I can take over

Copy link
Member

Choose a reason for hiding this comment

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

It's used by the worker to make sure we don't register to LK Cloud until everything is ready

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

davidzhao and others added 5 commits February 9, 2026 12:07
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Chenghao Mou <chenghao.mou@livekit.io>
Users can now pass plain dicts like record={"logs": False} without
importing RecordingOptions. Missing keys default to True.
@theomonnom theomonnom merged commit 48d163a into main Feb 16, 2026
17 of 18 checks passed
@theomonnom theomonnom deleted the dz/recording-options branch February 16, 2026 02:26
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.

3 participants