[asterisk-dev] [Code Review]: Ensure that CDRs for a caller in a Queue who is not answered is NO ANSWER

[Leif Madsen]

On 10/08/12 01:34 AM, Tilghman Lesher wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2064/
>
>
>     On August 9th, 2012, 3:06 p.m., *Matt Jordan* wrote:
>
>         Bumping the review.
>
>     On August 9th, 2012, 3:17 p.m., *Tilghman Lesher* wrote:
>
>         The problem is that you're only looking at the simplest case.  Suppose a call comes in, they talk to a person, and then they get transferred to a Queue, and they hang up before the Queue is answered (in any of the scenarios above).  Your essentially saying that the billsecs up until that time don't count and that the CDR should therefore both indicate "NO ANSWER", which is patently false, as well as recording some billsecs, because the call was, in fact, answered.  That will confuse all sorts of billing systems, and thus is really of the type of fix that shouldn't go in unless you account for problems like this; well, no, shouldn't go in, period.  There are too many call scenarios where trying to manipulate this value after the call was answered will be problematic and will break billing systems.  In other words, don't touch it.
>
>     On August 9th, 2012, 3:27 p.m., *Paul Belanger* wrote:
>
>         This was the reason for my previous comment.  I was under the impression that we no longer did any CDR fixes, and recommended people use CEL, simple because we could never ensure it would not break other scenarios.
>
>     On August 9th, 2012, 3:47 p.m., *Matt Jordan* wrote:
>
>         The assumption that we 'no longer did any CDR fixes' is incorrect.  CDRs have not been deprecated.  Would I like to deprecate them?  Sure - but I don't think that's a valid option at this time.  If I'm wrong in that then by all means, send an e-mail to the -users list telling everyone that CDRs are deprecated and see what response you get.
>
>         What we aren't going to attempt to do is define behavior that cannot be defined given the limitations imposed by CDRs.
>
>         So what behavior is undefined?  Well, one area that is certainly undefined in CDRs is what happens in transfer scenarios.  We've never claimed that the resulting CDR record in a blind or attended transfer is going to account for all of the actions that happen between the various channels.
>
>         So let's look at the case of an attended transfer as Tilghman outlined without this patch:
>
>         Phone A calls Phone B.  Phone B transfers Phone A into a Queue with Phone C as a member (unpaused) and Phone D as a member (paused).  Phone A hangs up.
>
>         Do you get a billsec value?  Yup.  Do you get a disposition?  Yup - BUSY in this case.  That hardly makes any sense, but - of course - CDR behavior in transfer scenarios is undefined.
>
>         Lets try this one.  Phone A calls Phone B.  Phone B transfers Phone A into a Queue with Phone C as a member and Phone D as a member, both unpaused.
>
>         Now its even worse: if there's an Answer() before the Queue, you'll get a CDR with a disposition of ANSWERED - if there isn't an Answer() before the Queue, you get a disposition of NOT ANSWERED.  In both cases, however, you still have a billsec value.
>
>         So Tilghman's problem already exists without this patch.
>
>         I'm not claiming to fix the case of transfers, nor am I trying to 'fix' all issues with CDRs.  In this case, however, the simple behavior of CDRs in Queue with multiple agents has multiple behaviors, none of which are predictable.  I'm not sure how this patch doesn't at least make the simpler case consistent.  Since that behavior has been defined with respect to a parallel Dial, matching that behavior with Queue makes some sense.
>
>     On August 9th, 2012, 3:51 p.m., *Matt Jordan* wrote:
>
>         I should amend my previous comment - in the case of the transfer (which as I said, is on its face undefined behavior), the call will be "ANSWERED" regardless of the Answer application before the Queue.  That happens even if no one answers Phone B in the Queue (which is probably not desirable either).
>
>         It does record BUSY if one of the Queue members was paused.
>
> I do understand and commiserate with your interest in fixing legitimate bugs.  But from my perspective and experience with prior "fixes" to the CDR subsystem, anything you do in this regard will be pilloried, when someone upgrades for security and gets your fix which breaks their billing system.  It's why we tread very lightly around the CDR subsystem and why we encourage people to use the CEL subsystem when writing new billing systems.
>
> Quite simply put, I don't think your desire for consistency (and especially not your proposed solution) outweighs the danger of breaking production systems in the middle of a release cycle.  It would seem to me that the only desired consistency would be to make the disposition ANSWERED even when queue members are paused (or more correctly, never to backtrack from the ANSWERED disposition to NO ANSWER, BUSY, or CONGESTION, when it comes to setting disposition at the end of a Queue).  I still think you should follow Paul's recommendation to only modify trunk, not any release branches.

I would have to agree here. Changing in the middle of the release cycle 
doesn't seem to outweigh the chances of breaking someones billing 
software. Personally, there are 2 things that should happen:

1) when a new major branch is created (1.8, 10, 11, 12, etc...) that 
CDRs are locked for that release. Even if they are wrong, at least they 
remain predicable from point release to point release, allowing 
implementers to work around any potential problems.

2) this is exactly the reason that, if CDRs are not going to be 
deprecated any time soon, that a suite of tests should be created to 
define and identify as many situations as possible. That way at the very 
least, transfers and other scenarios *are* defined, which then allows 
the project to define what is accepted. Those who are reliant on certain 
scenarios could then provide back test scenarios to make sure their 
situations are accounted for.

+1 from me on this being a trunk only (Asterisk 11 probably OK at this 
point as well) change.

A backport of the patch can certainly be provided in the team branches 
if you wanted to provide that.

-- 
Leif Madsen
http://www.oreilly.com/catalog/asterisk