[asterisk-dev] [Code Review] This patch implements CEL in trunk.

Fri Dec 19 16:11:01 CST 2008

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/105/#review271
-----------------------------------------------------------

In general, I am very concerned with how the CEL modules have been done.  There is mass code duplication between them and the existing CDR modules.  I'm wondering if a better approach would be to just add the CEL handlers into the existing CDR modules to avoid duplicating the common code for configuration handling and so forth.

/trunk/apps/app_celgenuserevent.c
<http://reviewboard.digium.com/r/105/#comment565>

    These headers do not appear to be accurate.

/trunk/apps/app_celgenuserevent.c
<http://reviewboard.digium.com/r/105/#comment566>

    Please convert to xmldocs

/trunk/apps/app_celgenuserevent.c
<http://reviewboard.digium.com/r/105/#comment567>

    Isn't this done automatically?

/trunk/apps/app_celgenuserevent.c
<http://reviewboard.digium.com/r/105/#comment568>

    Fix the return value to use AST_MODULE_LOAD_SUCCESS and friends

/trunk/apps/app_queue.c
<http://reviewboard.digium.com/r/105/#comment569>

    This should actually be DEVICE_STATE, not DEVICE_STATE_CHANGE

/trunk/cdr/cdr_radius.c
<http://reviewboard.digium.com/r/105/#comment570>

    ?

/trunk/cdr/cdr_sqlite.c
<http://reviewboard.digium.com/r/105/#comment571>

    Unrelated change?

/trunk/cel/cel_csv.c
<http://reviewboard.digium.com/r/105/#comment572>

    I'm not sure there is any benefit in having cel_csv since there is also cel_custom.

    The only reason we have cdr_csv and cdr_custom is for historical and backwards compatibility reasons.

/trunk/cel/cel_custom.c
<http://reviewboard.digium.com/r/105/#comment573>

    coding guidelines.

/trunk/cel/cel_custom.c
<http://reviewboard.digium.com/r/105/#comment574>

    Having to allocate a channel every time we log an event is quite expensive.  Perhaps we should use a thread local dummy channel for this purpose, to optimize things a bit.

/trunk/cel/cel_custom.c
<http://reviewboard.digium.com/r/105/#comment575>

    use NULL, not 0

/trunk/cel/cel_custom.c
<http://reviewboard.digium.com/r/105/#comment576>

    Reduce indentation:

    if (load_config()) {
       return DECLINE;
    }

    .. stuff ..

/trunk/cel/cel_manager.c
<http://reviewboard.digium.com/r/105/#comment577>

    coding guidelines

/trunk/cel/cel_manager.c
<http://reviewboard.digium.com/r/105/#comment578>

    spaces after casts (coding guidelines)

/trunk/cel/cel_manager.c
<http://reviewboard.digium.com/r/105/#comment579>

    NULL, not 0

/trunk/cel/cel_odbc.c
<http://reviewboard.digium.com/r/105/#comment581>

    This module needs to be re-worked.  It should be using res_odbc, not using ODBC directly.

/trunk/cel/cel_odbc.c
<http://reviewboard.digium.com/r/105/#comment580>

    explicit initialization not necessary

/trunk/cel/cel_pgsql.c
<http://reviewboard.digium.com/r/105/#comment582>

    ast_debug()

/trunk/cel/cel_pgsql.c
<http://reviewboard.digium.com/r/105/#comment583>

    ast_free() instead of free()

- Russell

On 2008-12-19 11:28:20, Steve Murphy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/105/
> -----------------------------------------------------------
> 
> (Updated 2008-12-19 11:28:20)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> CEL stands for Channel Event Logging.
> 
> (It's big, so brace yourself!)
> 
> It's like the Manager interface, but a little
> more specialized to report events that would
> be used to form CDRs or call activity logs.
> It logs to a set of backends much like CDR's
> do. Higher performance event loggers can be
> written on the CEL base, that would provide
> better thruput for generating higher-level
> event-based logging, like CDR's.
> 
> It's based in turn on Russell's event facility,
> and expands on its base, and shares in its
> advantages. For instance, the 
> ast_event_subscribe() func has been modified
> to accept a "name" argument for the subscription,
> for CLI viewing.
> 
> It also introduces a new channel field, the
> 'linkedid' field, that has 'viral' properties;
> when two channels interact, the oldest linkedid
> value of the two will override the younger. The 
> linkedID field starts being set to the uniqueID 
> field. All the channel drivers 'new' routines will
> also now accept a linkedid argument, in the
> which, if provided, will override the linkedid
> field. This would happen, for instance, in the
> Dial() app, when a peer channel is created 
> for the purpose of a call. From it's creation,
> that peer channel will have the same linkedid
> as the calling channel.
> 
> The linkedid field will be generated (by default)
> on all CEL logged events, and can be used to tie
> multi-legged calls and perhaps even call trees
> formed by 3-ways and transfers.
> 
> Brian Degenhardt of SwitchVox formed a snazzy
> real-time call logging facility using the CEL
> text logging backends.
> 
> Analogs to the CDR backends were formed for
> CEL event logging. It's been a while since they
> were originally formed, and they may profit 
> from an overhaul to re-derive them from the 
> CDR backends. The TDS backend has been
> recently re-derived, as the old version no
> longer compiles against the newer TDS libs.
> The odbc_adaptive interface has not been
> adapted for CEL, but this is always an option
> for the future.
> 
> hangupsource is also now being recorded into
> a channel field by that name.
> 
> A long and perhaps tedious design doc about
> CEL is in doc/cel-doc.tex
> 
> I added 'peeraccount', and 'linkedid' to the
> existing CDR code; these may be useful to
> folks who want to continue arranging deck chairs
> on the CDR Titanic.
> 
> Peeraccount: when two channels are bridged,
> they each update their peeraccount field with
> the accountcode of their partner in their
> respective 'peeraccount' channel fields.
> The CDR field is updated, and a new manager
> event is generated. The peeraccount info
> is reported in CEL events.
> 
> 
> Although reviewers may have issues of their own,
> my major questions are these:
> 
> 1. Should I just eliminate the cel-csv interface,
> and let everyone use the cel-custom interface if
> they want text logging?
> 
> 2. Should I bring in the odbc-adaptive stuff?
> 
> 3. Should I re-derive all the backends from
> the cdr backends as they exist today?
> 
> 4. Are the current tracked events sufficient
> for CDR generation which I'm specifying
> now in team/murf/RFCs?
> 
> 
> Diffs
> -----
> 
>   /trunk/Makefile 165819 
>   /trunk/apps/app_celgenuserevent.c PRE-CREATION 
>   /trunk/apps/app_chanisavail.c 165819 
>   /trunk/apps/app_dial.c 165819 
>   /trunk/apps/app_directed_pickup.c 165819 
>   /trunk/apps/app_followme.c 165819 
>   /trunk/apps/app_meetme.c 165819 
>   /trunk/apps/app_minivm.c 165819 
>   /trunk/apps/app_parkandannounce.c 165819 
>   /trunk/apps/app_queue.c 165819 
>   /trunk/apps/app_voicemail.c 165819 
>   /trunk/cdr/cdr_radius.c 165819 
>   /trunk/cdr/cdr_sqlite.c 165819 
>   /trunk/cel/Makefile PRE-CREATION 
>   /trunk/cel/cel_csv.c PRE-CREATION 
>   /trunk/cel/cel_custom.c PRE-CREATION 
>   /trunk/cel/cel_manager.c PRE-CREATION 
>   /trunk/cel/cel_odbc.c PRE-CREATION 
>   /trunk/cel/cel_pgsql.c PRE-CREATION 
>   /trunk/cel/cel_radius.c PRE-CREATION 
>   /trunk/cel/cel_sqlite.c PRE-CREATION 
>   /trunk/cel/cel_sqlite3_custom.c PRE-CREATION 
>   /trunk/cel/cel_tds.c PRE-CREATION 
>   /trunk/channels/chan_agent.c 165819 
>   /trunk/channels/chan_alsa.c 165819 
>   /trunk/channels/chan_console.c 165819 
>   /trunk/channels/chan_dahdi.c 165819 
>   /trunk/channels/chan_features.c 165819 
>   /trunk/channels/chan_gtalk.c 165819 
>   /trunk/channels/chan_h323.c 165819 
>   /trunk/channels/chan_iax2.c 165819 
>   /trunk/channels/chan_jingle.c 165819 
>   /trunk/channels/chan_local.c 165819 
>   /trunk/channels/chan_mgcp.c 165819 
>   /trunk/channels/chan_misdn.c 165819 
>   /trunk/channels/chan_nbs.c 165819 
>   /trunk/channels/chan_oss.c 165819 
>   /trunk/channels/chan_phone.c 165819 
>   /trunk/channels/chan_sip.c 165819 
>   /trunk/channels/chan_skinny.c 165819 
>   /trunk/channels/chan_unistim.c 165819 
>   /trunk/channels/chan_usbradio.c 165819 
>   /trunk/channels/chan_vpb.cc 165819 
>   /trunk/configs/cel.conf.sample PRE-CREATION 
>   /trunk/configs/cel_custom.conf.sample PRE-CREATION 
>   /trunk/configs/cel_manager.conf.sample PRE-CREATION 
>   /trunk/configs/cel_odbc.conf.sample PRE-CREATION 
>   /trunk/configs/cel_pgsql.conf.sample PRE-CREATION 
>   /trunk/configs/cel_sqlite3_custom.conf.sample PRE-CREATION 
>   /trunk/configs/cel_tds.conf.sample PRE-CREATION 
>   /trunk/doc/cel-doc.tex PRE-CREATION 
>   /trunk/doc/celdriver.tex PRE-CREATION 
>   /trunk/funcs/func_cdr.c 165819 
>   /trunk/funcs/func_channel.c 165819 
>   /trunk/funcs/func_odbc.c 165819 
>   /trunk/include/asterisk/cdr.h 165819 
>   /trunk/include/asterisk/cel.h PRE-CREATION 
>   /trunk/include/asterisk/channel.h 165819 
>   /trunk/include/asterisk/event.h 165819 
>   /trunk/include/asterisk/event_defs.h 165819 
>   /trunk/include/asterisk/utils.h 165819 
>   /trunk/main/Makefile 165819 
>   /trunk/main/ast_expr2.h 165819 
>   /trunk/main/ast_expr2.c 165819 
>   /trunk/main/asterisk.c 165819 
>   /trunk/main/cdr.c 165819 
>   /trunk/main/cel.c PRE-CREATION 
>   /trunk/main/channel.c 165819 
>   /trunk/main/cli.c 165819 
>   /trunk/main/devicestate.c 165819 
>   /trunk/main/dial.c 165819 
>   /trunk/main/event.c 165819 
>   /trunk/main/features.c 165819 
>   /trunk/main/logger.c 165819 
>   /trunk/main/manager.c 165819 
>   /trunk/main/pbx.c 165819 
>   /trunk/main/taskprocessor.c 165819 
>   /trunk/res/res_agi.c 165819 
> 
> Diff: http://reviewboard.digium.com/r/105/diff
> 
> 
> Testing
> -------
> 
> bmd has built a reporting system using it; I have run
> several simple tests to ensure updating with merges
> hasn't broken it, and made appropriate fixes.
> 
> 
> Thanks,
> 
> Steve
> 
>