Add DisconnectReason to ParticipantInfo.
#788
Conversation
🦋 Changeset detectedLatest commit: d4cd579 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
the command to add a changeset would be the CLI will guide you through the steps. For this repo it's best to select "all packages" to be affected by protocol changes. |
| @@ -131,6 +131,7 @@ message ParticipantInfo { | |||
| bool is_publisher = 13; | |||
| Kind kind = 14; | |||
| map<string, string> attributes = 15; | |||
| DisconnectReason disconnect_reason = 16; | |||
There was a problem hiding this comment.
any chance of potentially including insight in to whether the participant is attempting a reconnect ?
I think users have been asking about this in the past.
There was a problem hiding this comment.
That is a transient state. Will think more about it. This reason would really be valid only when state = DISCONNECTED. Reconnect indication would be orthogonal I think. What's your thinking on how that should work?
There was a problem hiding this comment.
Thinking more about the reconnecting indication.
reconnect = 1 if successful should be (mostly) transparent. But, that is the case where server clearly knows there is an attempt to resume. Adding an indication for what should be transparent feels a bit odd.
A full reconnect looks like a new connection to the server. If the participant exists in the room and a full reconnect happens, server could indicate that the participant is attempting another connection (but that would not work in cloud if connecting to different node and that node does not have the old participant's remote participant, would require sending a message to check, but that would impact every join).
Would be good to see what users want and figure out how to do that.
There was a problem hiding this comment.
Just wanted to raise the thought, but don't think it's a pressing thing. Don't remember hearing requests about this for a while now.
| @@ -131,6 +131,7 @@ message ParticipantInfo { | |||
| bool is_publisher = 13; | |||
| Kind kind = 14; | |||
| map<string, string> attributes = 15; | |||
| DisconnectReason disconnect_reason = 16; | |||
There was a problem hiding this comment.
Could it be set by SIP to indicate the call termination reason?
I'm thinking about the following mapping:
- Hangup ->
CLIENT_INITIATED - No response ->
UNAVAILABLE(new) - Busy ->
REJECTED(new)
There was a problem hiding this comment.
Sure. If that is Participant specific, I think it should be okay. Thoughts on that @davidzhao .
Currently, internally server has these reasons for participant close - https://github.com/livekit/livekit/blob/5d554867102d8b3bee6e8865a85cae5513c19c62/pkg/rtc/types/interfaces.go#L84 and it gets mapped to DisconnectReason here - https://github.com/livekit/livekit/blob/5d554867102d8b3bee6e8865a85cae5513c19c62/pkg/rtc/types/interfaces.go#L169.
We can make more participant close reasons and map them to reasons like you mention above.
We do have participant kind now. Maybe, each kind has more specific reasons.
Question: I do not know enough about how SIP participant works. How would the SIP participant be conveyed the reason for its close? Actually, I guess I should first learn how the SIP participant gets closed now.
There was a problem hiding this comment.
Currently it just disconnects from the room as usual (or is remove from the room by API). So I'll need to update the SDK to expose the disconnect reason (or at least some of them).
There was a problem hiding this comment.
Got it. Client leaving currently sends this
protocol/protobufs/livekit_rtc.proto
Line 39 in a77139e
DisconnectReason -> ParticipantCloseReason and back to DisconnectReason, but that is okay 🙂 )
There was a problem hiding this comment.
I think we can add these as new disconnect reasons for SIP.
There was a problem hiding this comment.
Thank you @davidzhao .
@dennwc Please let me know when you add these to LeaveRequest. We can then change the server to do the mapping. Maybe call the reasons with a SIP_ prefix for UNAVAILABLE and REJECTED. I do not particularly like prefixing with specific like that, but the un-prefixed version is very generic. So, that could be confusing.
There was a problem hiding this comment.
yeah hangup makes sense.. but for the rest, how about keeping it the original SIP reasons instead of mapping? this way we wouldn't need to prefix either
No response -> NO_RESPONSE
Busy -> USER_BUSY (not sure about the USER_, just wanted to disambiguate from server busy)
|
@dennwc Going to merge this for now. We can add more reasons in a subsequent PR. @davidzhao Please let us know if you have further thoughts on SIP disconnect reason Denys mentioned and if you have ideas on how to do that? Enhance |
* Add `DisconnectReason` to `ParticipantInfo`. * generated protobuf * Add changeset. * patch changeset --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@lukasIO Last time, I did not create a changeset. Is there a specific step to create that? Do I need to run
pnpm changesetin the repo?