[Asterisk-code-review] res pjsip session Added rtcp stats result vector into the se... (asterisk[master])

[Joshua C. Colp]

Joshua C. Colp has posted comments on this change. ( https://gerrit.asterisk.org/10899 )

Change subject: res_pjsip_session Added rtcp stats result vector into the session
......................................................................


Patch Set 5: Code-Review-1

(1 comment)

I don't think in off-nominal error cases the stats should be saved. I think it is only when a media session is terminated after being used that the stats in regards to it should be saved. As I previously said the way it is now can result in stats for RTP sessions that were never used, as well as stats for the same RTP session at different points in time.

As a consumer of the functionality I think trying to use the data would be problematic in its current state. You would need to go through the data, correlate any duplicates, determine if any should be ignored, and then it would be useful. Without doing so it can lead you to wrong interpretations. For example if stats for an RTP instance that was never used were present and I saw the statistics in the vector I might wonder if there was a problem - but in reality there wasn't, because the stream was never actually used.

https://gerrit.asterisk.org/#/c/10899/5/include/asterisk/res_pjsip_session.h
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/#/c/10899/5/include/asterisk/res_pjsip_session.h@144
PS5, Line 144: 	/*! \brief Related sip session */
             : 	struct ast_sip_session *sip_session;
The reason I mentioned passing this in, instead of storing it is because it alters the API and ripples out to different places as you've done. It also changes the reference to be a bit circular (thankfully without a real reference). A media state is supposed to be isolated and not know/care about the session it is stored on.

I think it is perfectly reasonable that the save function require a session to be passed in instead.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib25c2d3fc4da084aecfde2a82c1b1d733bd64fa5
Gerrit-Change-Number: 10899
Gerrit-PatchSet: 5
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Fri, 08 Feb 2019 15:26:12 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190208/c6d55545/attachment-0001.html>