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

Wed Dec 24 16:45:21 CST 2008

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > 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.

Yeah, me too, and I sat and thought on this for a while, and ended doing it this way because:
  1. While they are similar in flow and procedure, they operate on two different sets of data.
  2. Attempting to generalize the data will pull common routines into the core (so both mods can use it), and
     tend to obfuscate the original code.
  3. The real thrust of CEL is not really the backends; When the CEL2CDR module is ready, very few people
     will probably be motivated to use the backends, except as a failsafe backup. And, even in that
     case, I would only plan on supporting the cel_custom backend. I didn't want to spend
     the majority of my time working on the minority modules. 

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/apps/app_celgenuserevent.c, lines 1-27
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1977#file1977line1>
> >
> >     These headers do not appear to be accurate.

done

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/apps/app_celgenuserevent.c, lines 37-43
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1977#file1977line37>
> >
> >     Please convert to xmldocs

done

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/apps/app_celgenuserevent.c, lines 62-64
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1977#file1977line62>
> >
> >     Isn't this done automatically?

Maybe somewhere in the dim recesses of time, back in antiquity, 
it didn't. I removed it.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/apps/app_celgenuserevent.c, line 85
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1977#file1977line85>
> >
> >     Fix the return value to use AST_MODULE_LOAD_SUCCESS and friends

done. But, boy, is there a lot of work to do... almost no app returns this!

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, lines 6735-6736
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1985#file1985line6735>
> >
> >     This should actually be DEVICE_STATE, not DEVICE_STATE_CHANGE

done.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cdr/cdr_sqlite.c, lines 183-188
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1988#file1988line183>
> >
> >     Unrelated change?

Yes, an unrelated change. I guess I got irritated enough with the cdr sqlite loading... that I fixed it in this branch. Removed. I can review the cdr code later.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cdr/cdr_radius.c, lines 194-198
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1987#file1987line194>
> >
> >     ?

Hmmm.. Can't remember why I made this change. Removed.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_csv.c, lines 19-28
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1990#file1990line19>
> >
> >     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.

I've thought on this myself. Well, that did it; removed the cel_csv stuff.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_custom.c, lines 83-100
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1991#file1991line83>
> >
> >     coding guidelines.

put in a space in both places.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_custom.c, lines 126-135
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1991#file1991line126>
> >
> >     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.

Or make it so the var substitution code doesn't require a channel to work?

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_custom.c, line 163
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1991#file1991line163>
> >
> >     use NULL, not 0

done.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_custom.c, lines 169-187
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1991#file1991line169>
> >
> >     Reduce indentation:
> >     
> >     if (load_config()) {
> >        return DECLINE;
> >     }
> >     
> >     .. stuff ..

done.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_manager.c, lines 94-115
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1992#file1992line94>
> >
> >     coding guidelines

space after if added.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_manager.c, lines 157-159
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1992#file1992line157>
> >
> >     spaces after casts (coding guidelines)

done

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_manager.c, line 218
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1992#file1992line218>
> >
> >     NULL, not 0

done.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_odbc.c, line 22
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1993#file1993line22>
> >
> >     This module needs to be re-worked.  It should be using res_odbc, not using ODBC directly.

I'll drop this module, and use odbc_adaptive instead.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_odbc.c, line 79
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1993#file1993line79>
> >
> >     explicit initialization not necessary

Well, since I'm dropping this, this should be moot.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_pgsql.c, line 248
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1994#file1994line248>
> >
> >     ast_free() instead of free()

done, for all occurrences.

> On 2008-12-19 16:11:01, Russell Bryant wrote:
> > /trunk/cel/cel_pgsql.c, lines 193-194
> > <http://reviewboard.digium.com/r/105/diff/1/?file=1994#file1994line193>
> >
> >     ast_debug()

done, for all occurrences

- Steve

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

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
> 
>