[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] [Code Review] 2462: Rework CDRs for Asterisk 12
From: "Matt Jordan" <reviewboard () asterisk ! org>
Date: 2013-04-30 20:53:51
Message-ID: 20130430205351.12965.57161 () hotblack ! digium ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2462/
-----------------------------------------------------------
(Updated April 30, 2013, 8:53 p.m.)
Status
------
This change has been discarded.
Review request for Asterisk Developers.
Bugs: ASTERISK-21196
https://issues.asterisk.org/jira/browse/ASTERISK-21196
Repository: Asterisk
Description
-------
This patch reworks CDRs for Asterisk 12. For more information on why and what the \
records should look like, please view the specification on the wiki: \
https://wiki.asterisk.org/wiki/display/AST/Asterisk+12+CDR+Specification
Since the Bridge API migration resulted in all CDR code in features.c no longer being \
applicable, and the model of masquerades during transfers now no longer occurs \
(mostly), the vast majority of complex CDR code sprinkled throughout Asterisk was \
either wrong, or would never be executed. Rather than piling the CDR code back into \
the Bridging API, this patch instead builds CDRs off of messages received over the \
Stasis-Core message bus. However, as a result, all direct manipulation of CDRs by \
channel drivers, applications, and the Asterisk core is now highly suspect and most \
likely null and void.
This patch rips most of the existing code out.
Messages received over Stasis-Core are first dispatched by top level message \
handlers. The handlers job is to (a) find the CDRs that are affected by the message, \
(b) dispatch the message to the CDRs, and (c) based on the statuses, determine if \
more CDRs are needed.
Each CDR has a virtual table that determines what effect the message has on it. \
Depending on the state of the CDR, the message may be dropped; invalid; or it may \
alter the state of the CDR. CDRs transition states when they need to, and the top \
level message handlers generally don't care what state the CDR is in.
CDRs exist in a chain. As a channel does stuff, additional CDRs may be appended to \
the chain, and there may be multiple active CDRs at any given moment in time.
As a result of all of this, ForkCDR, NoCDR, and ResetCDR have essentially been \
rewritten and simplified greatly. The need for ForkCDR is significantly less, as \
additional CDRs are created between all pairs of channels in multi-party and transfer \
situations.
A new CDR function, CDR_PROP, has been added that lets the dialplan override who is \
the Party A when two channels enter into a bridge together. This function also lets \
you disable/re-enable CDRs on a channel, which precludes the need for NoCDR and one \
of ResetCDR's options.
Finally, a set of unit tests have been written that cover the existing functionality.
This patch is not complete - there is still some additional work to do and testing to \
perform. The following needs to be covered under subsequent reviews:
* Linkedid propagation. As CEL will need this as well, this will most likely become a \
core concept (if possible)
* Park. A unit test exists for this, however, the holding bridge is not yet handled \
properly. This is rather trivial, as most of the Bridge state logic simply needs to \
be avoided if a holding bridge is entered.
* Call Pickup. We need the Stasis Message for this before this can be completed.
* Attended transfers into an application. We need the Stasis Message for this too.
* Quite a few BUGBUG comments in app_queue (which needs the Dial messages - see \
ASTERISK-21551) and the media channel (see ASTERISK-21713)
Hopefully, this patch is far enough along that the approach can be verified and major \
bugs can be commented on. Ideally, this would go into bridge_construction so that the \
Test Suite running against it can be updated.
A few open questions:
* What do we want to do with peeraccount? In general, this property on the channel \
needs to be set by the bridging API, but much like the BRIDGEPEER channel variable, \
has issues in reasonably complex scenarios. Right now, the peeraccount is passed \
through the CDRs by virtue of the CDR engine knowing who the channel is bridged with, \
but inspecting this value through a function becomes problematic since we can't \
return a single value in multi-party bridges. CEL will also need an answer to this \
question. (There is also some bridging logic that applies a channel's accountcode to \
who its bridged with if the other channel doesn't have an accountcode; this does need \
to be performed by the bridging layer)
* There is certainly room for debate with where the userfield, accountcode, and \
amaflags should live. Currently that is CDR, channel, channel. The channel userfield \
is generally unused. These could all be combined; however, that forces some parity \
between CEL userfield and CDR userfield that didn't used to exist.
Diffs
-----
team/group/bridge_construction/CHANGES 387018
team/group/bridge_construction/UPGRADE.txt 387018
team/group/bridge_construction/addons/cdr_mysql.c 387018
team/group/bridge_construction/addons/chan_ooh323.c 387018
team/group/bridge_construction/apps/app_authenticate.c 387018
team/group/bridge_construction/apps/app_cdr.c 387018
team/group/bridge_construction/apps/app_dial.c 387018
team/group/bridge_construction/apps/app_disa.c 387018
team/group/bridge_construction/apps/app_dumpchan.c 387018
team/group/bridge_construction/apps/app_followme.c 387018
team/group/bridge_construction/apps/app_forkcdr.c 387018
team/group/bridge_construction/apps/app_osplookup.c 387018
team/group/bridge_construction/apps/app_queue.c 387018
team/group/bridge_construction/bridges/bridge_softmix.c 387018
team/group/bridge_construction/cdr/cdr_adaptive_odbc.c 387018
team/group/bridge_construction/cdr/cdr_csv.c 387018
team/group/bridge_construction/cdr/cdr_custom.c 387018
team/group/bridge_construction/cdr/cdr_manager.c 387018
team/group/bridge_construction/cdr/cdr_odbc.c 387018
team/group/bridge_construction/cdr/cdr_pgsql.c 387018
team/group/bridge_construction/cdr/cdr_radius.c 387018
team/group/bridge_construction/cdr/cdr_syslog.c 387018
team/group/bridge_construction/cdr/cdr_tds.c 387018
team/group/bridge_construction/cel/cel_manager.c 387018
team/group/bridge_construction/cel/cel_radius.c 387018
team/group/bridge_construction/cel/cel_tds.c 387018
team/group/bridge_construction/channels/chan_agent.c 387018
team/group/bridge_construction/channels/chan_dahdi.c 387018
team/group/bridge_construction/channels/chan_h323.c 387018
team/group/bridge_construction/channels/chan_iax2.c 387018
team/group/bridge_construction/channels/chan_local.c 387018
team/group/bridge_construction/channels/chan_mgcp.c 387018
team/group/bridge_construction/channels/chan_sip.c 387018
team/group/bridge_construction/channels/chan_skinny.c 387018
team/group/bridge_construction/channels/chan_unistim.c 387018
team/group/bridge_construction/funcs/func_callerid.c 387018
team/group/bridge_construction/funcs/func_cdr.c 387018
team/group/bridge_construction/funcs/func_channel.c 387018
team/group/bridge_construction/include/asterisk/cdr.h 387018
team/group/bridge_construction/include/asterisk/cel.h 387018
team/group/bridge_construction/include/asterisk/channel.h 387018
team/group/bridge_construction/include/asterisk/config_options.h 387018
team/group/bridge_construction/include/asterisk/stasis_channels.h 387018
team/group/bridge_construction/include/asterisk/strings.h 387018
team/group/bridge_construction/include/asterisk/test.h 387018
team/group/bridge_construction/include/asterisk/time.h 387018
team/group/bridge_construction/main/asterisk.c 387018
team/group/bridge_construction/main/bridging.c 387018
team/group/bridge_construction/main/cdr.c 387018
team/group/bridge_construction/main/cel.c 387018
team/group/bridge_construction/main/channel.c 387018
team/group/bridge_construction/main/channel_internal_api.c 387018
team/group/bridge_construction/main/cli.c 387018
team/group/bridge_construction/main/config_options.c 387018
team/group/bridge_construction/main/features.c 387018
team/group/bridge_construction/main/manager.c 387018
team/group/bridge_construction/main/manager_channels.c 387018
team/group/bridge_construction/main/pbx.c 387018
team/group/bridge_construction/main/stasis_channels.c 387018
team/group/bridge_construction/main/test.c 387018
team/group/bridge_construction/main/utils.c 387018
team/group/bridge_construction/res/res_agi.c 387018
team/group/bridge_construction/res/res_config_sqlite.c 387018
team/group/bridge_construction/res/res_monitor.c 387018
team/group/bridge_construction/res/res_stasis.c 387018
team/group/bridge_construction/tests/test_cdr.c PRE-CREATION
Diff: https://reviewboard.asterisk.org/r/2462/diff/
Testing
-------
A lot of unit testing.
This patch allows the existing Test Suite tests to be updated to cover CDRs in \
Asterisk 12 as well.
Thanks,
Matt Jordan
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2462/">https://reviewboard.asterisk.org/r/2462/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#e0e0e0" width="100%" cellpadding="8" style="border: 1px gray \
solid;"> <tr>
<td>
<h1 style="margin-right: 0.2em; padding: 0; font-size: 10pt;">This change has been \
discarded.</h1> </td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Matt Jordan.</div>
<p style="color: grey;"><i>Updated April 30, 2013, 8:53 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21196">ASTERISK-21196</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">This patch reworks CDRs for Asterisk 12. For more information on why and \
what the records should look like, please view the specification on the wiki: \
https://wiki.asterisk.org/wiki/display/AST/Asterisk+12+CDR+Specification
Since the Bridge API migration resulted in all CDR code in features.c no longer being \
applicable, and the model of masquerades during transfers now no longer occurs \
(mostly), the vast majority of complex CDR code sprinkled throughout Asterisk was \
either wrong, or would never be executed. Rather than piling the CDR code back into \
the Bridging API, this patch instead builds CDRs off of messages received over the \
Stasis-Core message bus. However, as a result, all direct manipulation of CDRs by \
channel drivers, applications, and the Asterisk core is now highly suspect and most \
likely null and void.
This patch rips most of the existing code out.
Messages received over Stasis-Core are first dispatched by top level message \
handlers. The handlers job is to (a) find the CDRs that are affected by the message, \
(b) dispatch the message to the CDRs, and (c) based on the statuses, determine if \
more CDRs are needed.
Each CDR has a virtual table that determines what effect the message has on it. \
Depending on the state of the CDR, the message may be dropped; invalid; or it may \
alter the state of the CDR. CDRs transition states when they need to, and the top \
level message handlers generally don't care what state the CDR is in.
CDRs exist in a chain. As a channel does stuff, additional CDRs may be appended to \
the chain, and there may be multiple active CDRs at any given moment in time.
As a result of all of this, ForkCDR, NoCDR, and ResetCDR have essentially been \
rewritten and simplified greatly. The need for ForkCDR is significantly less, as \
additional CDRs are created between all pairs of channels in multi-party and transfer \
situations.
A new CDR function, CDR_PROP, has been added that lets the dialplan override who is \
the Party A when two channels enter into a bridge together. This function also lets \
you disable/re-enable CDRs on a channel, which precludes the need for NoCDR and one \
of ResetCDR's options.
Finally, a set of unit tests have been written that cover the existing functionality.
This patch is not complete - there is still some additional work to do and testing to \
perform. The following needs to be covered under subsequent reviews:
* Linkedid propagation. As CEL will need this as well, this will most likely become a \
core concept (if possible)
* Park. A unit test exists for this, however, the holding bridge is not yet handled \
properly. This is rather trivial, as most of the Bridge state logic simply needs to \
be avoided if a holding bridge is entered.
* Call Pickup. We need the Stasis Message for this before this can be completed.
* Attended transfers into an application. We need the Stasis Message for this too.
* Quite a few BUGBUG comments in app_queue (which needs the Dial messages - see \
ASTERISK-21551) and the media channel (see ASTERISK-21713)
Hopefully, this patch is far enough along that the approach can be verified and major \
bugs can be commented on. Ideally, this would go into bridge_construction so that the \
Test Suite running against it can be updated.
A few open questions:
* What do we want to do with peeraccount? In general, this property on the channel \
needs to be set by the bridging API, but much like the BRIDGEPEER channel variable, \
has issues in reasonably complex scenarios. Right now, the peeraccount is passed \
through the CDRs by virtue of the CDR engine knowing who the channel is bridged with, \
but inspecting this value through a function becomes problematic since we can't \
return a single value in multi-party bridges. CEL will also need an answer to this \
question. (There is also some bridging logic that applies a channel's accountcode \
to who its bridged with if the other channel doesn't have an accountcode; this \
does need to be performed by the bridging layer)
* There is certainly room for debate with where the userfield, accountcode, and \
amaflags should live. Currently that is CDR, channel, channel. The channel userfield \
is generally unused. These could all be combined; however, that forces some parity \
between CEL userfield and CDR userfield that didn't used to exist.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">A lot of unit testing.
This patch allows the existing Test Suite tests to be updated to cover CDRs in \
Asterisk 12 as well.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>team/group/bridge_construction/CHANGES <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/UPGRADE.txt <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/addons/cdr_mysql.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/addons/chan_ooh323.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_authenticate.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_cdr.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_dial.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_disa.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_dumpchan.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_followme.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_forkcdr.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_osplookup.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/apps/app_queue.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/bridges/bridge_softmix.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_adaptive_odbc.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_csv.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_custom.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_manager.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_odbc.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_pgsql.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_radius.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_syslog.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cdr/cdr_tds.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cel/cel_manager.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cel/cel_radius.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/cel/cel_tds.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_agent.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_dahdi.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_h323.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_iax2.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_local.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_mgcp.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_sip.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_skinny.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/channels/chan_unistim.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/funcs/func_callerid.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/funcs/func_cdr.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/funcs/func_channel.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/cdr.h <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/cel.h <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/channel.h <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/config_options.h <span \
style="color: grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/stasis_channels.h <span \
style="color: grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/strings.h <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/test.h <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/include/asterisk/time.h <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/asterisk.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/bridging.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/cdr.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/cel.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/channel.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/channel_internal_api.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/cli.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/config_options.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/features.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/manager.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/manager_channels.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/pbx.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/stasis_channels.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/test.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/main/utils.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/res/res_agi.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/res/res_config_sqlite.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/res/res_monitor.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/res/res_stasis.c <span style="color: \
grey">(387018)</span></li>
<li>team/group/bridge_construction/tests/test_cdr.c <span style="color: \
grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2462/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic