[Asterisk-code-review] res/res_ari: Added ARI resource /ari/channels/{channelId}/rtp_statistics (...asterisk[master])

[sungtae kim]

sungtae kim has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11083 )

Change subject: res/res_ari: Added ARI resource /ari/channels/{channelId}/rtp_statistics
......................................................................


Patch Set 6:

(9 comments)

Hi Joshua,

Thank you for your kind reviewing. I've fixed it. :)

https://gerrit.asterisk.org/#/c/11083/5//COMMIT_MSG 
Commit Message:

https://gerrit.asterisk.org/#/c/11083/5//COMMIT_MSG@9 
PS5, Line 9: Added ARI resource for channel statisctics.
> statistics
Ooops!


https://gerrit.asterisk.org/#/c/11083/5//COMMIT_MSG@10 
PS5, Line 10: GET /ari/channels/{channelId}/stats : It returns given
> I don't think this is the right name for this. […]
Agree. I've fixed it. :)


https://gerrit.asterisk.org/#/c/11083/5/channels/chan_pjsip.c 
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/#/c/11083/5/channels/chan_pjsip.c@1275 
PS5, Line 1275: 	j_res = ast_json_array_create();
> I don't think it should be the responsibility of the channel driver to construct this information. […]
Agree, I've just didn't know about the 'glue'. Now, I've removed all the channel driver stuffs. :)


https://gerrit.asterisk.org/#/c/11083/5/include/asterisk/channel.h 
File include/asterisk/channel.h:

https://gerrit.asterisk.org/#/c/11083/5/include/asterisk/channel.h@829 
PS5, Line 829: 	struct ast_json * (* get_rtp_stats)(struct ast_channel *chan, const int type);
> I don't think this is necessary. […]
Thanks, that was what I wanted. :) Removed all the channel driver stuffs. :)


https://gerrit.asterisk.org/#/c/11083/5/main/rtp_engine.c 
File main/rtp_engine.c:

https://gerrit.asterisk.org/#/c/11083/5/main/rtp_engine.c@3748 
PS5, Line 3748: 	j_res = ast_json_object_create();
> This can fail.
Agree, fixed it to set the value when it was succeed.


https://gerrit.asterisk.org/#/c/11083/5/main/rtp_engine.c@3750 
PS5, Line 3750: 	ast_json_object_set(j_res, "txcount", ast_json_integer_create(stats->txcount));
> All of these various calls can also fail, leaving the resulting object in an undefined incomplete st […]
Same live above. :)


https://gerrit.asterisk.org/#/c/11083/5/res/ari/resource_channels.c 
File res/ari/resource_channels.c:

https://gerrit.asterisk.org/#/c/11083/5/res/ari/resource_channels.c@1966 
PS5, Line 1966: 
> If the underlying channel is not RTP should this return a different response code?
Agree, fixed to 403 when the channel is not suitable tech type, and added more 404 response cases.


https://gerrit.asterisk.org/#/c/11083/5/rest-api/api-docs/channels.json 
File rest-api/api-docs/channels.json:

https://gerrit.asterisk.org/#/c/11083/5/rest-api/api-docs/channels.json@1677 
PS5, Line 1677: 			"path": "/channels/{channelId}/stats",
> Per my commit message comment - I don't think this is the right name for this
Agree, fixed it. :)


https://gerrit.asterisk.org/#/c/11083/5/rest-api/api-docs/channels.json@1678 
PS5, Line 1678: 			"description": "RTP stats on a channel",
> I think this description could be better: […]
Yes, your description is better. Fixed it. :)

But, changed the RTCP to RTP. 

Get RTP statistics information for RTP on a channel



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11083
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I4343eec070438cec13f2a4f22e7fd9e574381376
Gerrit-Change-Number: 11083
Gerrit-PatchSet: 6
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 00:22:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190305/52173ecb/attachment-0001.html>