Adding UML diagrams in API provider perspective#122
Adding UML diagrams in API provider perspective#122stroncoso-quobis merged 10 commits intocamaraproject:mainfrom
Conversation
stroncoso-quobis
left a comment
There was a problem hiding this comment.
Hi @teikuran ,
First of all: thanks for the documentation, and sorry for my late response.
Here are some comments and analysis:
Use cases
There are two cases for a WebRTC API Invoker:
The API Invoker is an application on the end-user device
The API Invoker is a server of a third-party application service provider
[...] This call flow illustrates the former case.
I'm not really sure that they are so different. Or at least on both diagrams, currently on the repo and the proposed ones. Basically seems to be the Notification Rely. Current ones include it, with a different name, into the Device dialog, but the participants involved are exactly the same. Maybe it is a service worker or a separate service on a VM or a cloud service like Firebase, but, from my point of view, it does not really affect the use case.
My proposition here is to merge both into one unique set of diagrams. Your proposition is well-structured with the Markdown document, so I will vote to go with it.
Subscriptions
We agreed to use only deviceId to configure the subscription. Any later command, call or register that involves that deviceId, should include a reference to the registrationId. In that case, the Notifycation System can identify which specific registration or call is affected on the event.
This is properly represented, looks great.
Question: Why the message [30], detailed on the markdown, uses the registrationId? Shouldn't it rely on the deviceId? The event generated should indicate in the details of the registrationId affected. Should we keep this event-registration per register flexibility?
Register
Marking SIP REGISTER as optional in the diagrams looks good. But, just to clarify: A regsitration action (message 22) is MANDATORY to activate the device or phone number as reachable on the network. Isn't it?
My question here: Don't you think that the registration should include a phoneNumer on the request? Otherwise we are assuming that the phoneNumber comes from the onboarding process, and I am not sure if we can rely on that. In fact the response include a regInfo but we did not specify on the documentation where this phone number comes from. Maybe it is a different bug 🐛 but comes to my mind reviewing the diagrams.
Overview
- In general terms it looks great, no changes requested, simply comments and questions.
- I will add the activate/deactivate each component when receive/emit a message. Ex.: messages 4 to 8 look weird without activation of any entity, nevertheless all of them are related. I suggest marking all activities.
- Checking the events that you notify back or not, it comes to my mind the table that we elaborated at
webrtc voice api.md. Take a look at it, maybe it's worth a review. At least, I want to review maybe some things. - I miss a summary diagram, something easy to attract the developer. With the agreed changes of the API, to include on the main doc. Something simple and clean:
- First, you declare a
sinkand subscribe to all events using yourdeviceId - Then, register to active routing to your device and notify the network that you are reachable (at this
deviceId) - Lastly, interact with calls and registrations using the other webrtc endpoints along with their specific object IDs:
webrtc-call-handlingusingmediaSessionIdwebrtc-registrationusingregistrationId
- First, you declare a
Summary questions
- Should we merge diagrams?
- Should we keep the
config.registrationIdon the event subscription POST? - Should we allow calls without registration? Emergency is one use case, but it is not unique. An autodialer is another interesting use case that does not need to receive calls.
- Should we allow the subscription to other lines events? (ex.: an event watcher for multiple company lines)
- Should we include a
phoneNumberfield on the registration? - Should we review
webrtc voice api.mdevent list?
--
Thanks a lot for your work. And again, sorry for the late reply.
Best regards
|
thank you for your comment. here is the response.
I think we need a further study. We do not disagree with your point, with the understanding that both the current (existing and our proposed) UML diagrams are drafted based on the scenario where “the API Invoker is an application on the end-user device” (UNI case). In terms of impact on the API specification, there may not be any significant difference compared to the case where “the API Invoker is a server of a third-party application service provider” (NNI case). However, there could be some differences in the SIP flow. For example, in the NNI case, the server invokes the registration API, but the SIP registration itself may not be necessary. With this understanding, before making any decision on how to capture the flow (merge or not) in the repository, we would like to first describe these two use cases separately, and then, if necessary, draft UML diagrams for each. We are also currently working on preparing the use case descriptions for that purpose.
No. we are fine with using a per-device subscription i.e., rely on deviceId. Since the event report includes the affected registrationId, using the deviceId instead of the registrationId is better. But for that we should update webrtc-events.yaml becasue currently REGISTRATION_ENDS_SUBSCRIPTION example shows the use of registrationId.
Yes we think so.
To our understanding phoneNumer can be derived based on the access token.
We see your point. Don't have strong opnion but let's see. We would be happy if you could update the diagrams so after initial ver is uploaded.
So could please elaborate a bit more about your conern . But if you are referring to the mapping review between the Status and SIP Response, then ... we would like to prefer to decouple that from this PR.
Got it.
I see your point. We dont have strong opinion on this. Let's see.
Is this about multiple subscrptions being created in one step(as in new Commonalities)? |
…l_origination.png due to README update
…l_origination.wsd due to README update
…istration.png due to README update
…istration.wsd due to README update
…istration_and_expiration_subscription.md due to README update
…_for_call_events.png due to README update
…_for_call_events.wsd due to README update
…scription_for_call-related_events.md due to README update
|
To summarize the updates:
|
stroncoso-quobis
left a comment
There was a problem hiding this comment.
Approving MR
More content to come
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
The current UML diagram has been drafted from the perspective of how the API client invokes the API. The uploaded diagrams are drafted from the API provider’s perspective. By having a clear view of API invocation from both the client and provider perspectives, we can better identify any issues with the current APIs.
Which issue(s) this PR fixes:
Fixes #109
Changelog input