Add KV metadata for rooms and participants #728
Conversation
dennwc
requested review from
biglittlebigben,
davidzhao,
frostbyte73,
keepingitneil and
shishirng
🦋 Changeset detectedLatest commit: 627bc19 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 |
protobufs/livekit_models.proto
Outdated
| message KVMetadata { | ||
| // KV uses Key type from the corresponding model. | ||
| // Integer keys allow discovering key names from the protobuf itself. | ||
| map<int32,string> kv = 1; |
There was a problem hiding this comment.
What would be the easiest way for a customer to define their own keys? Or is this not a design goal?
There was a problem hiding this comment.
Good question! It's not a goal, since they already have an opaque metadata field that they control. This one is solely for our own use.
There was a problem hiding this comment.
As discussed on the call, we will now allow user-defined keys too and thus keys will be strings. I updated the PR to reflect that.
|
using a strictly typed metadata field would be better.
the ability to rapidly evolve our apis is far less valuable than providing a stable, consistent, intuitive api. apis we expose to customers need to be more stable and ergonomic than apis we consume internally. while we can quickly coordinate internal changes the communication overhead for external changes scales poorly. thrashing and breaking customer products is damaging to our brand. |
|
After today's call we decided to keep it simple and go with a |
|
@paulwe although I completely agree with your argument about the strict schema, I believe the use case here is a bit different. We do not want any critical LiveKit-related data (or even metadata) here. We only want to pass some optional metadata for the end user to read. A good example would be a provider-specific call ID for SIP. We definitely don't want a The second reason is that passing separate metadata fields across the system is brittle. KV metadata acts as a single container and can be propagated as a whole, even if the service in the middle cannot directly unmarshal it. |
that's not how apis work... https://www.hyrumslaw.com
this seems fine if it's a 3rd party integration?
no, it's not. by default the protobuf unmarshal/marshal propagates unknown fields. eg. message TestFoo {
string a = 1;
string b = 2;
}
message TestBar {
string a = 1;
}foo := &api.TestFoo{
A: "a",
B: "b",
}
fooBuf := must.Get(proto.Marshal(foo))
bar := &api.TestBar{}
must.Do(proto.Unmarshal(fooBuf, bar))
barBuf := must.Get(proto.Marshal(bar))
foo2 := &api.TestFoo{}
must.Do(proto.Unmarshal(barBuf, foo2))
fmt.Println(protojson.Format(foo2)){
"a": "a",
"b": "b"
}
making a single catch all metadata container means making a k/v store part of our api. we do not want to do this. we especially do not want to mix user values and internally generated values in one place because this will create a mess for synchronization. a single unstructured blob of metadata with last write wins semantics and limited space should be enough for storing a pointer to data in customer's dbs... |
Using semi anonymous fields here seem fine to me as, as Dennys mentions, these are informational, service specific fields. I'm afraid that if we specify typed fields, we'll end up reproducing large swatch of other provider APIs in our protocol. wrt having out own k/v store: I think the design stems from the fact that we believe there is a need for us to provide this functionality (in a limited way) to our customers, as is it likely they will need synchronization semantics across several metadata producers who change the value fairly often to communicate a change in a participant behavior. If you believe implementing this reliably is too much engineering or maintenance work and we'd rather have each of our customers figure out how to do this with the semantics they need, I think that's something we can discuss indeed, but there is real value in providing this as part of our API IMHO. |
if we're passing this data as k/v pairs we'll be doing this anyway only in a half-baked, unmaintainable way. if you believe implementing strict types is too much engineering or maintenance work and we'd rather have each of our customers figure out what values are available i think that's something we can discuss... anyway, this seems to be happening... the client should implement a map type api with message MetadataLog {
string key = 1;
TimedVersion version = 2;
oneof value {
bool deleted = 3;
string string = 4;
}
}last write wins. updates are unordered and eventually consistent. total k/v size (keys+values) should be limited. message RoomMetadataLog {
repeated MetadataLog log = 1;
}
message ParticipantMetadataLog {
string participant_id = 1;
repeated MetadataLog log = 2;
}room and participant metadata should not be part of either room or participant. resending the entire room or participant every time a key changes is not a good use of signal capacity. message KVSyncRequest {
message ParticipantMetadataLog {
string participant_identity = 1;
repeated MetadataLog log = 2;
}
repeated MetadataLog room = 1;
repeated ParticipantMetadataLog participants = 2;
}
message KVSyncResponse {}
service KV {
rpc Sync(KVSyncRequest) returns (KVSyncResponse);
}k/v api requests received without versions will be assigned one by the service. server apis can implement batch operations eg |
True. But that law also implies that any change, including deprecation will break someone. I think you would agree that one cannot move forward without (occasional) breaking changes.
That's the problem - it's not an integration with Twilio. We do expose SIP protocol, but there are thousands of providers that speak it. We do not want to model all of them in our API. All we need is to map a few to this KV metadata and let users add the rest (if they care).
Indeed, it will preserve unknown fields in that Maps are simpler in this regard, since they can be serialized to JSON (or any other encoding), rebuilt manually, stored to DB natively, etc. It would be a problem for
To be clear, we are not proposing a full-featured KV API. It's merely a replacement for existing opaque
I think you specifically referenced it here, right?
I share your concerns here. Initial proposal was to make it immutable, so that the first time the participant is created, the metadata will be locked. Or that we make a separate permission in the token to only allow our internal services to modify it. But during today's sync an argument was made that maybe we could allow users to store their own data there. Personally I don't have a strong opinion here, especially since I'm not familiar with the way For the short term, we only need a way to attach metadata during room/participant creation. That will cover current customer's needs. If users will end up overwriting keys with our prefix - I think that's their problem. Luckily, current permissions on metadata only allow modifying your own metadata, so potential damage is very limited. On our side, we need to enforce (via code review) that we do not read this metadata, or at least don't act on it. Rule seems pretty clear: if a field plays a role in server logic, it must be structured properly. As for the updates, would it be realistic to only have a single guarantee: that a new key can be inserted and it will be visible to all peers eventually? That's the main use case: we want to insert metadata once, and we assume user may also want to insert something (e.g. some identifier). So maybe we could only allow insertions and no updates? That would very efficiently prevent anyone (including us) from storing any important state there. WDYT? |
|
@keepingitneil Would be great to have your input here too. Do you plan to have any kind of metadata for Agents that is attached to rooms/participants? And if so, what operations do you expect on this data? |
At the moment we don't have any livekit owned metadata planned for agents themselves. User-defined metadata could be interesting for agent devs, but not essential and also applies to all clients (not just agent clients). I'm not up to speed with the latest agent durability/orchestration design but I trust @biglittlebigben and @paulwe have already advocated appropriately if that design makes use of metadata. If I were to opine here 😉 - I like the commit log design. An additional, livekit_owned:true/false field could be added to the log message to prevent users (and livekit) from accidentally using metadata they are not supposed to use. edit: or instead of livekit_owned: boolean, a namespace: string |
the existing metadata is a mutable blob store. participant metadata can be set in the token and updated using the signal and server apis. room metadata can be created and updated using the server api. this is table stakes for any new metadata implementation...
to guarantee first write wins we have to globally serialize writes. implementing last write wins only requires storing the timestamp of the last update and propagating it to all replicas... either way, once we add a k/v store customers will wonder why it doesn't support durability, acid, transactions, atomic ops, large blob storage, whatever. we should focus on our core offerings rather than over engineering a solution for delivering headers
if we want to add immutable values supplied during room/token creation we should call these fields something completely different to avoid confusion. repeating prefixes is wasteful and inelegant and there is no reason sip, agent, and user supplied values should share a single keyspace. |
This is actually a pretty good argument to not allow user metadata in KV 🤔
That's exactly what I want to avoid. Let's find the minimal possible contract that doesn't require a transaction log or ACID guarantees.
That's the theory, yes. But we do sync
Any alternative name you would suggest? I agree that seeing
Definitely wasteful for storage/transmission, but it's a well established practice for end-user APIs (e.g. K8S annotations, Redis, etcd). We could change the storage details, but I think it's the most convenient way to access it from user's perspective. Please take into account that customers are wondering why we cannot deliver this sooner, since it's "only" forwarding metadata. It might not be reasonable to return to them saying "we want to develop KV with TX log for this, so ETA is a >month". We should try hard to find a practical compromise here, in my opinion. I think the main assumption here is that our services (e.g. SIP) or their services already have a primary DB of some kind. So we don't need strong guarantees for KV. We need a way to tag our models with some identifiers, that presumably do not change over time. I'm really tempted to say that we should just merge these KV updates in the order they arrive. If someone requests stronger guarantees we should point them in other direction: generate a UID, set it in KV once (no updates!) and store their state in other DB. Later, if we do decide to build a proper KV from it, we could change the wire format to the TX log you proposed. We can deliver this fast enough and it still gives room for future improvement, if needed. |
we already globally serialize writes to participant state. the media node hosting the participant serializes writes and propagates those changes to replicas. if we can limit this new feature to participant metadata only the solution is much simpler than if we need to build a consistent solution for rooms and participants.
annotations, labels, tags, flags, or description? and i would prefix them like |
This can actually be the answer. We don't need room metadata for SIP use case. |
|
OK then, the consensus appears to be that KV can be SIP specific. I will close this PR in favor of a new one. Thanks everyone for the feedback! |
As discussed a few times earlier, we need a way to set (immutable?) metadata on rooms and participants that we control.
For SIP specifically, this means any call-related metadata (calling/called number, call id, etc). It definitely doesn't make sense to put this info directly on Participant.
I considered adding a
SIPMetadatamessage as a field of Participant, but it sounds like an overkill. Nothing in our stack will directly read these values. This is why I think a map would be nicer. It allows any LK sub-system (SIP, Agents, etc) to quickly add metadata fields which will be propagated through the whole stack automatically. It's also possible to put experimental metadata earlier and move it to protocol later.The current plan is to have a
map<string,string> kv_metadatafield and have a naming convention for the keys (e.g.livekit.sip.callID).