RestComm / ussdgateway

RestComm USSD Gateway
http://www.restcomm.com/
GNU Affero General Public License v3.0
88 stars 102 forks source link

Add number of seconds for a USSD session in CDRs #72

Closed deruelle closed 6 years ago

deruelle commented 7 years ago

Each CDR should contain the number of seconds the USSD Dialog was alive for

abdulazizali77 commented 6 years ago

@vetss Hi Sergey, some initial questions from me: briefly looking at the org.mobicents.ussdgateway.slee.cdr package, am i correct to assume that

  1. we start measuring the dialog usage time at the point USSDCDRState.init() is called (of which one instance is inside ChildSbb.onProcessUnstructuredSSRequest() but could be anywhere else too)
  2. we stop measuring the dialog usage time when we persist the record? It seems that this is mostlly done with USSDCDRState.setRecordStatus() inside ChargeInterface.createRecord() 2b. will/wont we ever call USSDCDRState.setRecordStatus() without actually wanting to persist the record later? (seems improbable since we are interested in all state changes i guess)
vetss commented 6 years ago

Hello @abdulazizali77

USSDCDRState.init() is a proper place to put into USSDCDRState a current time as a dialog start. When you write a CDR record you can calculate a dialog duration as a difference between current time and USSDCDRState.init() time.

I will not be able (the most probably) to check your PR this week (because of vacations).

abdulazizali77 commented 6 years ago

Thank you @vetss

vetss commented 6 years ago

Hello @abdulazizali77

I have checked your PR https://github.com/RestComm/ussdgateway/pull/75

My comments:

1) you initialize state.setDialogStartTime(DateTime.now()); not only in onProcessUnstructuredSSRequest() (that affects PULL case) but also for a PUSH case. Check from which places USSDCDRState.init() is invoked, we need to cover all 3 places. 2) we need to update a USSD GW manual for a list of CDR fields, check for examle a chapter "6.2.3. CDR Table Structure" and other CDR related topics 3) What will happen if a customer uses a jdbc database and he has applied your last update ? I suppose he will has an issue that a new field has not been added into a database. The best approach is that we have some script for updating of database. Please investigate how it is possible. 4) you need to make a separate test for database based CDRs how records are strired into a database.

I can not add your commit till we fix the mentioned issues.

abdulazizali77 commented 6 years ago

@vetss thanks Sergey for the review,

  1. Ive added the start time for the two PUSH cases, but since there are no actual dialogs for PUSH cases, should we rename DIALOG_DURATION to something not related to the tcap dialog?
  2. Added in configuring and running sections. Added to the docbook module even though it looks like the docbook files are never processed.
  3. Ive added version check and changed the create logic. Please advise if this is too much
  4. I dont see any preexisting CDRGeneratorSbb unittests even for Plain, what should the tests cover? unit or integrated/end to end (in SipClientSbb, HttpClientSbb)?
vetss commented 6 years ago

Hello @abdulazizali77 I will check your PR, thanks for your work.

One note for

I dont see any preexisting CDRGeneratorSbb unittests even for Plain, what should the tests cover? unit or integrated/end to end (in SipClientSbb, HttpClientSbb)?

I meant just to run USSD GW at your local server with some simulators / TC and cofiguring for database CDRs and check how well CDR will be stored (and database was created).

I also see another PR "Save Ussd response string to CDRs" , I beleive it is for CDRs also. Will you prepare CDR database update also for this case ? I mean may be better to make all updates for CDRs together to minimize of development efforts.

vetss commented 6 years ago

Hello @abdulazizali77

updates from https://github.com/RestComm/ussdgateway/pull/75 looks good for me.

1) You need to update only docs in "sources-ansiidoc", other sources are old, we do not need to update it. (It is not bad that you updates "sources", it is just not needed).

2) If we need of an update https://github.com/RestComm/ussdgateway/pull/77 (Save Ussd response string to CDRs) and thinking of it also demands database update may be we can update that PR also for a new CDR field and database scripts for adding of both fields together. Then we can test both fileds in a live server (plain and jdbc database CDRs) and accept PRs. @abdulazizali77 WDYT ?

abdulazizali77 commented 6 years ago

@vetss Hi Sergey -Have recreated a proper PR #79 from my fork -have retained the DocBook 'sources' changes -have added joda-time dependency to bootstrap -have squashed all commits -These are the db dml of my local test using hsqldb

INSERT INTO USSD_GW_CDRS VALUES('474dfa80-5c70-438a-ae0d-43c881a642fe',2,8,1,NULL,NULL,NULL,8,0,4,'9960639902','*519#',1,1,'111111',1,1,'222222',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'FAILED_CORRUPTED_MESSAGE','PULL','2017-09-20 18:47:59.879000000',1,1,166)
INSERT INTO USSD_GW_CDRS VALUES('a3538125-e9da-4b44-aeef-8ea6345ee020',2,8,1,NULL,NULL,NULL,8,0,4,'9960639902','*519#',1,1,'111111',1,1,'222222',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'FAILED_CORRUPTED_MESSAGE','PULL','2017-09-20 18:48:20.728000000',2,2,27)
INSERT INTO USSD_GW_CDRS VALUES('4922e5fa-55e1-4a61-a6b7-0fd13dbe2ff7',2,8,1,NULL,NULL,NULL,8,0,4,'9960639902','*519#',1,1,'111111',1,1,'222222',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'SUCCESS','PULL','2017-09-20 18:50:10.997000000',3,3,3957)
INSERT INTO USSD_GW_CDRS VALUES('ee93ec95-0131-41d2-9389-bd519475b8d5',2,8,1,NULL,NULL,NULL,8,0,4,'9960639902','*519#',1,1,'111111',1,1,'222222',NULL,NULL,NULL,NULL,NULL,NULL,NULL,'FAILED_INVOKE_TIMEOUT','PULL','2017-09-20 18:53:26.537000000',4,4,25045)
vetss commented 6 years ago

Fixed by:

https://github.com/RestComm/ussdgateway/commit/630774a906980d261b42ea0c7b7a75ea530a4209 https://github.com/RestComm/ussdgateway/commit/fc7d1df060da25c04d60fe23550cce8055874261

vetss commented 6 years ago

Hello @abdulazizali77

Please check my comments in https://github.com/RestComm/ussdgateway/issues/76

abdulazizali77 commented 6 years ago

Latest results of local testing here JDBC https://github.com/RestComm/ussdgateway/issues/76#issuecomment-335776047 Plain text https://github.com/RestComm/ussdgateway/issues/76#issuecomment-335782993