[Home]

Summary:ASTERISK-19860: Wrong CDR duration values because of dependency to CDR write time especially in cdr batch mode
Reporter:Thomas Arimont (tomaso)Labels:
Date Opened:2012-05-11 08:26:25Date Closed:2012-08-24 08:41:24
Priority:MajorRegression?
Status:Closed/CompleteComponents:CDR/General
Versions:1.8.11.1 Frequency of
Occurrence		Frequent
Related
Issues:
Environment:Attachments:
Description:Our CDR statistis contain lots of items with wrong duration values where the CDR disposition is 'BUSY'.
Duration values in connection with the disposition 'BUSY' however should either
a) be set to (approx.) 0 if a call is rejected immediately (destination not found, channel unavailable, and so on)
or
b) be set to the elapsed ringing time if the call is rejected by the user/endpoint

I guess that someone has tried to resolve some problematical cases where a cdr end time was not written due to some reason.
This is done in the function ast_cdr_getvar() (cdr.c) by simply overwriting the cdr duration with a new calculated value based on the cdr write time as a substitute for the missing cdr end time.
Especially in cdr batch mode the actual time of writing of a cdr can widely differ from the (missing) end time and this finally leads to the inaccurate values.

{noformat}
void ast_cdr_getvar(struct ast_cdr *cdr, const char *name, char **ret, char *workspace, int workspacelen, int recur, int raw)
{
...
else if (!strcasecmp(name, "duration"))
snprintf(workspace, workspacelen, "%ld", cdr->duration ? cdr->duration : (long)ast_tvdiff_ms(ast_tvnow(), cdr->start) / 1000);

...
}
{noformat}
Comments:By: kwk (kwk) 2012-06-08 04:12:13.737-0500

To me, the calculation of CDR fields (e.g. duration, billsec) is not a backend (e.g. SQL, CSV) business. But if you naively look at the source code this way:

{noformat}
find . -name "*.[c|h]" -print0 | xargs -0 grep "cdr" | grep "duration"
{noformat}

you'll see in multiple places, that the duration (among other fields) is calculated in a backend:

addons/cdr_mysql.c
{noformat}
if (!strcasecmp(cdrname, "duration") &&
                                       (strstr(entry->type, "float") ||
                                       strstr(entry->type, "double") ||
                                       strstr(entry->type, "decimal") ||
                                       strstr(entry->type, "numeric") ||
                                       strstr(entry->type, "real"))) {

                                       snprintf(workspace, sizeof(workspace), "%lf",
                                               (double) (ast_tvdiff_us(cdr->end, cdr->start) / 1000000.0));

                                       if (!ast_strlen_zero(workspace)) {
                                               value = workspace;
                                       }
                               }
{noformat}

./cdr/cdr_adaptive_odbc.c
{noformat}
} else if (!strcasecmp(entry->cdrname, "duration")) {
                                                       snprintf(colbuf, sizeof(colbuf), "%lf",
                                                                               (double) (ast_tvdiff_us(cdr->end, cdr->start) / 1000000.0));

                                                       if (!ast_strlen_zero(colbuf)) {
                                                               colptr = colbuf;
                                                       }
                                               }

{noformat}

The cdr/csv.c on the other hand doesn't calculate the duration. In build_csv_record() it access the CDR's duration property directly and writes it to the buffer:

{noformat}
/* Duration */
       append_int(buf, cdr->duration, bufsize);
{noformat}

My proposal is to have a more consistent way of dealing with CDRs. The duration should be calculated in one place only, so that a CDR is ready to be written to whatever backend you want without further calculation. Otherwise you'll have different timing information depending on the backend and the frequency with which it is called (e.g. see batch mode).
By: Matt Jordan (mjordan) 2012-06-22 08:07:14.879-0500

I tend to agree that the CDR backends should not be manipulating the duration.

Unfortunately - at least in the case of cdr_adaptive_odbc - duration can be reported in fractional seconds.  In the CDR core, however, duration is always calculated as whole seconds - which means that, if someone has specified a column type for duration that implies fractional values - the backend has to recalculate the duration.

An ideal solution would be to redo the CDR core to record duration and billsec using a type suitable for all backends; however, that's a fairly intrusive change.  We've found that intrusive changes in CDRs - especially in release branches - is a risky business.

There is a patch on Review Board that addresses this issue for the core CDR backends:

https://reviewboard.asterisk.org/r/1996/