Add SIP analytics events #831
Conversation
🦋 Changeset detectedLatest commit: 54996f8 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 |
biglittlebigben
force-pushed
the
benjamin/SIP_events
branch
from
57cc671 to
47f810d
Compare
| string sip_call_id = 26; | ||
| SIPCallInfo sip_call = 27; | ||
| string sip_trunk_id = 28; | ||
| SIPInboundTrunkInfo sip_inbound_trunk = 29; |
There was a problem hiding this comment.
Is the SIP service responsible for sending analytics?
If so, we need to pass Trunk and Dispatch Rule infos via internal protocol to file these fields.
Or will the other service file them based on the IDs above?
There was a problem hiding this comment.
The current plan is to have livekit-server (OSS) and cloud-io (cloud) send these events. We will need some sort of "UpdateSIPCallState" internal call sip -> livekit/cloud-io to trigger the analytics event.
There was a problem hiding this comment.
Maybe worth declaring it in this PR as well?
protobufs/livekit_analytics.proto
Outdated
| SIP_CALL_ACCEPTED = 37; | ||
| SIP_CALL_STARTED = 38; | ||
| SIP_CALL_ENDED = 39; |
There was a problem hiding this comment.
does this differentiate between inbound and outbound calls? should we have a separate status for CONNECTING or is that what STARTED is? for example the flow for inbound:
- CONNECTING -> when sip picks up, but no user on the other side
- ACCEPTED -> when sip connects to end user.
and for outbound
- CONNECTING -> when we start dialing for outbound
- ACCEPTED -> when the other side picks up
There was a problem hiding this comment.
In the current proposal, PARTICIPANT_CREATED and CALL_ACCEPTED match the "CONNECTING" state you suggest in the outbound and inbound cases respectively. I guess ACCEPTED may be confusing as it means that the SIP server accepted it, not the room. Maybe use "CALL_INCOMING"?
SIP_PARTICIPANT_CREATED has the pro that it relates directly to an API call IMHO.
There was a problem hiding this comment.
Or CALL_ACCEPTED -> RINGING?
There was a problem hiding this comment.
got it. so accepted is for incoming only.. makes sense.. then I would vote for INCOMING, since it's a bit more explicit about that fact. wdyt?
protobufs/livekit_sip.proto
Outdated
| message SIPCallInfo { | ||
| string call_id = 1; | ||
| string trunk_id = 2; | ||
| string room_id = 3; // ID of the current/previous room published to |
There was a problem hiding this comment.
should we include the name as well?
Note to @dennwc, room creation is async, and it could take a few seconds before the room ID resolves.
protobufs/livekit_sip.proto
Outdated
| SIP_CALL_STARTING = 0; | ||
| SIP_CALL_ACTIVE = 1; | ||
| SIP_CALL_DISCONNECTED = 2; | ||
| SIP_CALL_STATUS_CALL_INCOMING = 0; |
There was a problem hiding this comment.
kind of a nit.. these enums are pretty long.. wdyt about using abbrevations like SIP_ or SCS_
There was a problem hiding this comment.
and since it could be confusing what each status means, wanna just put comments in here?
No description provided.