<p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/11083">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11083/5//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5//COMMIT_MSG@9">Patch Set #5, Line 9:</a> <code style="font-family:monospace,monospace">Added ARI resource for channel statisctics.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">statistics</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5//COMMIT_MSG@10">Patch Set #5, Line 10:</a> <code style="font-family:monospace,monospace">GET /ari/channels/{channelId}/stats : It returns given</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11083/5/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5/channels/chan_pjsip.c@1275">Patch Set #5, Line 1275:</a> <code style="font-family:monospace,monospace"> j_res = ast_json_array_create();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11083/5/include/asterisk/channel.h">File include/asterisk/channel.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5/include/asterisk/channel.h@829">Patch Set #5, Line 829:</a> <code style="font-family:monospace,monospace"> struct ast_json * (* get_rtp_stats)(struct ast_channel *chan, const int type);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11083/5/main/rtp_engine.c">File main/rtp_engine.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5/main/rtp_engine.c@3748">Patch Set #5, Line 3748:</a> <code style="font-family:monospace,monospace"> j_res = ast_json_object_create();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This can fail.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5/main/rtp_engine.c@3750">Patch Set #5, Line 3750:</a> <code style="font-family:monospace,monospace"> ast_json_object_set(j_res, "txcount", ast_json_integer_create(stats->txcount));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">All of these various calls can also fail, leaving the resulting object in an undefined incomplete state.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11083/5/res/ari/resource_channels.c">File res/ari/resource_channels.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5/res/ari/resource_channels.c@1966">Patch Set #5, Line 1966:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If the underlying channel is not RTP should this return a different response code?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11083/5/rest-api/api-docs/channels.json">File rest-api/api-docs/channels.json:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5/rest-api/api-docs/channels.json@1677">Patch Set #5, Line 1677:</a> <code style="font-family:monospace,monospace"> "path": "/channels/{channelId}/stats",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Per my commit message comment - I don't think this is the right name for this</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11083/5/rest-api/api-docs/channels.json@1678">Patch Set #5, Line 1678:</a> <code style="font-family:monospace,monospace"> "description": "RTP stats on a channel",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this description could be better:</p><p style="white-space: pre-wrap; word-wrap: break-word;">"Get RTCP statistics information for RTP on a channel"</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11083">change 11083</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/11083"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I4343eec070438cec13f2a4f22e7fd9e574381376 </div>
<div style="display:none"> Gerrit-Change-Number: 11083 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 05 Mar 2019 12:55:52 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>