Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for key/value attributes on Participant #733

Merged
merged 14 commits into from
Jun 20, 2024
Merged

Support for key/value attributes on Participant #733

merged 14 commits into from
Jun 20, 2024

Conversation

davidzhao
Copy link
Member

This picks up after #728

Key/value attributes lets us:

  • allow various components to provide more information about the participant (i.e. SIP including the phone number)
  • allow Agents to communicate their own status to the end user via RTC
  • enable applications to update specific fields without overriding others

davidzhao and others added 2 commits June 6, 2024 15:28
Key/value attributes lets us:
* allow various components to provide more information about the participant (i.e. SIP including the phone number)
* allow Agents to communicate their own status to the end user via RTC
* enable applications to update specific fields without overriding others
Copy link

changeset-bot bot commented Jun 6, 2024

🦋 Changeset detected

Latest commit: a4d0f89

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
Some errors occurred when validating the changesets config:
The package or glob expression "github.com/livekit/protocol" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@davidzhao davidzhao requested review from paulwe, dennwc, biglittlebigben and a team June 6, 2024 22:31
@paulwe
Copy link
Contributor

paulwe commented Jun 7, 2024

for sip we need a way to copy information about the caller from the sip api to the participant. this data is created once when the participant is created and never updated.

for agents we need a way to change the job config after the job is dispatched ie. update the voice or language. this config is initially copied from the job description to the job and can be modified while the job is running. ideally this would work the same way for both room and publisher jobs.

for sip i'm not sure we've considered the privacy implications of broadcasting pii to everyone in the room. caller information should not be part of the participant api. it does not change so we do not need to retransmit it and we should deliver it only to privileged participants. similarly agent config updates should not be delivered to every participant in a room. any private information the agent needs would be publicly visible.

for agents the proposed change does not provide a way to update room jobs or to update one job specifically. developers could implement their own namespaces and try to avoid collisions but this is kludgy...

we should define a new message type SIPCallerInfo and add a repeated field to ParticipantUpdate to deliver it. ParticipantPermission should have a new field can_read_sip_caller_info that enables the session to receive these updates.

for agents we should add signal and agents api methods for updating jobs. this can work the same as the proposed changes to the update participant apis but specify job rather than participant subjects. updates can be delivered to the agent worker ws and dispatched to the running job by the agent client framework.

@boks1971
Copy link
Contributor

boks1971 commented Jun 7, 2024

Sorry, did not read @paulwe 's comments before bonking this. I think this needs more discussion after reading his comments.

Copy link
Contributor

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

@davidzhao Looks good! For SIP we'll also need it in a few other places (like CreateSIPParticipant and DispatchRule). Should I edit this PR directly?

But @paulwe made a good point about a separate permission. Initially I thought that we could reuse hide_phone_number that we already have, but I guess users will want to have it both ways - hide for participants, but show for agents and other services.

So @paulwe do I understand correctly that you still propose to completely move SIP part out of Participant and deliver it separately as an update? Is there a way to deliver it with the first participant info, so that other services won't need to wait for a separate update to arrive?

@davidzhao
Copy link
Member Author

davidzhao commented Jun 8, 2024

I've synced with Paul. He had two concerns:

  1. privacy of always exposing the caller's number to everyone in the room

This is a good callout. The decision of exposing the phone number vs not should not be up to us, but it should be up to the developer.

  1. building specific APIs for each use case vs general APIs that are used for multiple use cases.

Paul was saying it would be more clear to have specific APIs for each use case. While I don't disagree, I think it becomes a burden on the client teams to implement a lot of different features. For that reason, I personally prefer general constructs.


@dennwc for 1, do you think this can be a SIP specific option? for example, a setting to indicate if a user's phone number should be anonymous. Perhaps it should be specified on the trunk or dispatch rule?

@dennwc
Copy link
Contributor

dennwc commented Jun 10, 2024

for example, a setting to indicate if a user's phone number should be anonymous. Perhaps it should be specified on the trunk or dispatch rule?

Yes, we already have a hide_phone_number on Dispatch Rule, so we can reuse it. Will need to add the same setting on CreateSIPParticipant too.

@dennwc
Copy link
Contributor

dennwc commented Jun 12, 2024

@davidzhao I added SIP-related attributes, should be good to go from my side.

Please check the attribute prefix for SIP, we won't be able to change it later.

livekit/attrs.go Outdated
// Names of participant attributes for SIP.
const (
// AttrSIPPrefix is shared for all SIP attributes.
AttrSIPPrefix = "lk.sip."
Copy link
Member Author

Choose a reason for hiding this comment

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

do you think the lk. namespace adds value here? IMO since we don't have any reserved namespace, it could just be tagged SIP?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for us not wanting a reserved namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

the mechanism for us to set these attributes is via token grants, we do not have a way to enforce that a particular component is SIP vs something else. Since the developer is responsible for issuing tokens, they should be "trusting" information that they are providing.

livekit/attrs.go Outdated Show resolved Hide resolved
@dennwc
Copy link
Contributor

dennwc commented Jun 12, 2024

One more note, there's no way currently to set attributes on participant creation, right?

@davidzhao
Copy link
Member Author

One more note, there's no way currently to set attributes on participant creation, right?

ah yeah.. I will add that in a few

@davidzhao
Copy link
Member Author

@dennwc I've added it here. I'll leave the branch open until server support is integrated.

@dennwc
Copy link
Contributor

dennwc commented Jun 13, 2024

Also worth adding a changeset.

@davidzhao
Copy link
Member Author

Also worth adding a changeset.

will do!

@lukasIO
Copy link
Contributor

lukasIO commented Jun 17, 2024

One thing I'm unsure about is using the same permission for both metadata and attributes.
From an application level perspective, I imagine it to be nice to be able to differentiate those two.
E.g. metadata could be used for things the participant is allowed to update directly, but attributes might make more sense in some cases to be controlled by the application's backend.

@davidzhao
Copy link
Member Author

One thing I'm unsure about is using the same permission for both metadata and attributes. From an application level perspective, I imagine it to be nice to be able to differentiate those two. E.g. metadata could be used for things the participant is allowed to update directly, but attributes might make more sense in some cases to be controlled by the application's backend.

I think for certain attributes, you do want the participant to update: i.e. their current state or preferences. Do you think it makes sense to have another permission canUpdateOwnAttributes ?

@lukasIO
Copy link
Contributor

lukasIO commented Jun 17, 2024

I think for certain attributes, you do want the participant to update: i.e. their current state or preferences.

yeah, totally agree, for certain use cases (esp. agents) that makes a lot of sense.

Do you think it makes sense to have another permission canUpdateOwnAttributes ?

that's what I was thinking, yeah. Not sure if it's worth it though, we'd probably want a separate signal message then as well just for the attributes...

@davidzhao
Copy link
Member Author

that's what I was thinking, yeah. Not sure if it's worth it though, we'd probably want a separate signal message then as well just for the attributes...

I guess the question is.. do we see any use cases where it makes sense to allow participants to update one, but not the other? If so it might be worth it to separate.

@dennwc
Copy link
Contributor

dennwc commented Jun 17, 2024

I tried integrating this change to SIP, and it looks like it will have to send attribute update at least once too. Some headers are available only later during the call, there's no way to bake them into the token. But all SIP attrs mentioned in this PR can be.

@lukasIO lukasIO mentioned this pull request Jun 19, 2024
@davidzhao
Copy link
Member Author

decided to keep the permissions the same, in order to avoid additional complexity without a use case that requires it. merging.

@davidzhao davidzhao merged commit e0c49d3 into main Jun 20, 2024
3 checks passed
@davidzhao davidzhao deleted the attributes branch June 20, 2024 05:27
@github-actions github-actions bot mentioned this pull request Jun 17, 2024
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.

6 participants