Sending device model over ffi for client telemetry#966
Sending device model over ffi for client telemetry#966MaxHeimbrock wants to merge 9 commits intomainfrom
Conversation
|
From sync: this is technically a breaking change since it changes the C ABI. However, given that other client info is passed this way, I would say we should do the same here for consistency. |
.changeset/sending_device_model_over_ffi_for_client_telemetry.md
Outdated
Show resolved
Hide resolved
ladvoc
left a comment
There was a problem hiding this comment.
Looking good so far, I've left a few questions and comments.
| capture_logs: bool, | ||
| sdk: *const c_char, | ||
| sdk_version: *const c_char, | ||
| device_model: *const c_char, |
There was a problem hiding this comment.
suggestion: Unlike SDK version, not all clients will be able to populate this field. We can store this as Option<String> on the Rust side, mapping a null pointer to Option::None.
There was a problem hiding this comment.
How do I do the mapping here?
I tried:
device_model: {
if device_model.is_null() {
None
} else {
let s = CStr::from_ptr(device_model).to_string_lossy().into_owned();
if s.is_empty() {
None
} else {
Some(s)
}
}
And that did not work when not filling the field from Unity side. I again end up with garbage in logs:
There was a problem hiding this comment.
We discussed that the proposed code is needed and clients can pass null as last parameter for device_model if they can not find out about the device.
xianshijing-lk
left a comment
There was a problem hiding this comment.
two nits, lgtm if you address them.
This change sends the device model in client telemetry when set from ffi, defaulting to 'Unknown' otherwise.
a0091d4 to
ec16a71
Compare
Send device_model in client telemetry if set from ffi.
|
I moved the field over to the general signaling options instead of the SDK options. |
This adds a parameter `livekit_ffi_initialize`, requiring all ffi clients to provide the device_model or null.
ladvoc
left a comment
There was a problem hiding this comment.
LGTM ✅. Note that FFI clients that do not provide device model will need to be updated to pass NULL in for the new parameter when updating to the latest FFI release.
This is probably a breaking change.
Send device_model if set from ffi, otherwise send "Unknown"
Alternatively we could add it to some room options proto, but this follows the same pattern as sdk and sdk version, which is also client telemetry and set in the
LiveKitInitializeof the ffi.Enables: livekit/client-sdk-unity#201