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

[Joshua Colp]

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

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


Patch Set 5: Code-Review-1

(9 comments)

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


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. This call is not channel statistics, and just calling it stats can make people assume it is. It should be named based on what it provides - which is RTP statistics, so perhaps rtp_statistics.


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. If it needs to change or more needs to be done, each channel driver that may implement this has to be changed. I also don't think a new callback is needed at all.


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. There already exists an RTP engine "glue" API which can be used to retrieve the RTP instances from channels. This is used by bridge_native_rtp to do its work. It could also be used by a common function to retrieve the RTP instances and construct the appropriate JSON in a common generic fashion.


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.


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 state.


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?


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


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:

"Get RTCP 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: 5
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: Tue, 05 Mar 2019 12:55:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190305/a3e22400/attachment-0001.html>