[Home]

Summary:ASTERISK-14189: [patch] No unique identifier for CDR
Reporter:nick_lewis (nick_lewis)Labels:
Date Opened:2009-05-22 08:45:00Date Closed:2009-11-03 15:26:36.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:CDR/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) cdrinstance.patch
( 1) cdr-sequence10.diff
( 2) cdr-sequence2.diff
( 3) cdr-sequence3.diff
( 4) cdr-sequence8.diff
( 5) cdr-sequence9.diff
Description:There is no unique identifier for a CDR. This makes it difficult to associate other things such as call monitor recordings with a CDR.
Each call does have a unique identifier which is stored in the CDR as "uniqueid" but there may be multiple CDRs representing one call. For example ResetCDR(w) saves a CDR and creates a new one for the call. Both CDR have the same "uniqueid".

****** ADDITIONAL INFORMATION ******

I propose a new read-only CDR field named something like "cdrinstance" that automatically increments after a ResetCDR(w) command. This can be used in conjunction with the "uniqueid" field to form a unique CDR identifier.
Comments:By: Russell Bryant (russell) 2009-05-22 08:49:05

Unfortunately, feature requests without patches are not accepted on the issue tracker.

If you would like to discuss this idea on the asterisk-dev list, that would be welcome.
By: nick_lewis (nick_lewis) 2009-05-26 11:51:22

Reopening with a patch
By: nick_lewis (nick_lewis) 2009-05-27 05:02:15

Tested ok with mysql cdr database and following dialplan macro:

[macro-record-enable]
exten => s,1,GotoIf($["${BLINDTRANSFER}" = ""]?check)
exten => s,n,ResetCDR(w)
exten => s,n,StopMonitor()
exten => s,n(check),AGI(recordingcheck)
exten => s,n,MacroExit()
exten => s,n(record),MixMonitor(${UNIQUEID}i${CDR(cdrinstance)}.wav)

Each transfer of a call creates a new CDR and a corresponding new callmonitor recording. The callmonitor recording filenames permit them to be linked unambiguously with the rows of a CDR listing thus permitting click to play functionality
By: Tilghman Lesher (tilghman) 2009-08-25 01:59:40

This needs to be discussed on the -dev list prior to implementing this feature.  Please send a new post to the asterisk-dev list, detailing the problem you're trying to solve and your proposed solution.  If there is consensus that your approach is best, this patch will be committed.
By: Leif Madsen (lmadsen) 2009-09-22 08:35:31

Is there any update here? Was that post ever sent to the mailing list? If so, I'd like someone to add a note with the link to the discussion (lists.digium.com).

Or if someone knows that the discussion resulted in not wanting to move this issue forward, then please close, or request to have it closed.

My question would be, "Why not just add an 'id' field and auto-increment?"
By: nick_lewis (nick_lewis) 2009-09-25 07:35:29

http://lists.digium.com/pipermail/asterisk-dev/2009-August/039457.html
(Unfortunately my mail client breaks the thread so you may need to search for posts entitled "No unique identifier for CDR" to follow it)
By: Matthew Nicholson (mnicholson) 2009-10-19 10:03:52

What was the consensus from that mailing list thread?
By: nick_lewis (nick_lewis) 2009-10-20 03:28:41

There was a consensus that a unique cdr id was needed. A couple of people did not understand why it could not just be an sql field (but usage reasons given in the thread). There was a suggestion that the cdrinstance be named something else such as seq or sequence and there were no objections to this. There was also a suggestion that the cdrinstance be unique to the call based linked-id field rather than channel based unique-id field (i.e. calls that transfer to a different source channel do not reset the cdrinstance). There was no objection to this.

Therefore I think there is a consensus in support of the approach of Atis Lezdins. (I do not know if he is planning to work on an implementation.)
By: Matthew Nicholson (mnicholson) 2009-10-21 12:11:46

The threads on gmane for reference:
http://thread.gmane.org/gmane.comp.telephony.pbx.asterisk.devel/36724
http://thread.gmane.org/gmane.comp.telephony.pbx.asterisk.devel/36753


By: Matthew Nicholson (mnicholson) 2009-10-21 13:54:31

I have reviewed the mailing list threads and given this some though.  Right now, I am thinking of implementing a 'sequence' field that will be incremented each time a CDR is created.  This way the value will be available in the dialplan.  The disadvantage of this approach is that the sequence value will not always be sequential.

The alternative is to generate the sequence value when the CDR is posted, but the sequence value would not be available in the dialplan with this approach.

I would like some feedback on this before I spend time working on an implementation.
By: Tilghman Lesher (tilghman) 2009-10-21 21:49:52

I think that's a good approach to take.  Any attempt to ensure that the extra part of the unique ID is sequential is likely to cause more trouble than it's worth.  Full speed ahead!
By: nick_lewis (nick_lewis) 2009-10-22 02:47:39

Such an approach would certainly meet my needs
By: Matthew Nicholson (mnicholson) 2009-10-26 14:48:38

Ok.  Try the patch that I just uploaded.
By: Matthew Nicholson (mnicholson) 2009-10-26 14:57:52

The additional CDR field I added is called "sequence".  It is available for back ends to use.  You probably want to test with the cdr-custom backend.
By: Sebastian Gutierrez (sum) 2009-10-26 15:48:08

what about have a UUID in the CDR table? this could be generated when the call start and be avaiable at the dialplan, also you don't have to worry about what is on the cdr table. this is the way I found easier to link a cdr record with a recorded file, I just have an app that generates the UUID and I set it to the channel variables each time I have a new call.
By: nick_lewis (nick_lewis) 2009-10-27 07:26:34

mnicholson
Thanks for the patch. For me the combination of channel "uniqueid" and cdr "sequence" is as good as a uuid. The patch to my eye though looks a bit over complicated. Why not always keep the sequence number unchanged during cdr duplication. In all but bridging this is the desired behaviour. In the case of a bridge_cdr why not do an atomic increment of the sequence number immediately after the duplication. Is there a reason why the increment must be part of the duplication function and why the increment is combined with sequence number initiation?
By: Matthew Nicholson (mnicholson) 2009-10-27 08:46:19

The sequence number must change sometimes during duplication because ForkCDR may call ast_cdr_dup, in which case the duplicated cdr should have a different sequence number.  On the other hand, CDR backends call ast_cdr_dup on records to get a copy they can work with.  Those records do not need a different sequence number.

The sequence number generation is tied to the channel so that each sequence number generated for that channel is unique.  If the sequence number was simply incremented for each CDR record, you have the possibility that a CDR record with an older sequence number will be duplicated.  That would lead to a duplicate sequence number.

Thinking about this further, with my implementation, it is still possible to get a duplicate sequence number if the channel used to generate the sequence number is not the same channel referenced by the uniqueid field in a CDR record.  One work around for this would be to fetch the channel via its uniqueid when generating sequence numbers.  This approach would fail if the channel has gone away when or if the channel is not yet in the channel list when we attempt to generate a sequence number.

It may be best to go with a true uuid after all.
By: nick_lewis (nick_lewis) 2009-10-27 09:32:37

Sorry I had forgotten about forkcdr

Rather than using the channel based "uniqueid" which can be swapped out during a source end transfer, why not use the recently introduced "linkedid" as the basis for the "sequence". This linked id reliably follows the call so does not suffer from the same problems. (originally suggested by Atis Lezdins)

I guess the current cdr->linkedid variable would need to become a pointer to a new linkedid structure containing the old linkedid variable and a sequence variable. Then

linkedid = cdr->linkedid;
cdr->sequence = ast_atomic_fetchadd_int(linkedid->sequence, 1));


The "linkedid"/"sequence" combination would work for me (as would uuid)
By: Matthew Nicholson (mnicholson) 2009-10-27 09:36:34

Something like that would work in theory.  Though I like the idea of an actual uuid.
By: nick_lewis (nick_lewis) 2009-10-27 10:50:25

Be it call sequence or uuid, I think there is a broader question regarding linkedid. This or a similar structure to represent the call would still be useful. It seems crazy holding cdr info against the channel and then having to duplicate it all every time a call transfer causes the channel to change. Couldnt channels just have references to the call they are currently part of and the new call structure then reference the individual call detail records (ast_cdr). Wouldnt this prevent a lot of faffing about during transfers etc
By: Matthew Nicholson (mnicholson) 2009-10-27 11:25:10

Yes, that design would be somewhat of an improvement.  That may not completely handle or cleanly handle cases where channels are bridged multiple times or involved in conferences or never bridged.

Tracking events that occur on channels solves all of these problems and this is how CEL is implemented.  In fixing this bug, I don't intend to duplicate CEL.
By: nick_lewis (nick_lewis) 2009-10-27 11:44:28

Humans seem to find it very easy to identify a "call" no matter how many or what type of bridges are involved. Some pbx use a thread per call rather than per channel (e.g. freeswitch) so I guess it must be possible even for conferences and unbridged calls.

Anyway I am drifting even further off topic. A uuid would be fine for me.
By: Matthew Nicholson (mnicholson) 2009-10-27 15:52:12

I just uploaded cdr-sequence3.diff.  This patch uses a global int to generate the sequence number.  I choose this approach after discussing the issue with some people here.  It should meet your uniqueness requirements and was simple to implement.
By: Matthew Nicholson (mnicholson) 2009-10-30 13:37:04

Please test the latest patch and report you results here.
By: Matthew Nicholson (mnicholson) 2009-11-02 10:33:11.000-0600

I did some testing for this patch, and while it does accomplish the task of generating a way to uniquely identify a CDR record, but I found an issue that may limit the usefulness of this issue for some.

In short, the CDR sequence number written to disk is different than the sequence number the dialplan sees (although it is the same as the sequence number on the 'h' exten).  Fixing that issue is going to be complicated because of the way asterisk generates CDRs for bridges.
By: nick_lewis (nick_lewis) 2009-11-02 11:03:56.000-0600

'Fraid it's no good for me

I need a $CDR[sequence] that can be read in the dialplan either immediately before or immediately after a ResetCDR(w) command and is the same as that written to disk
By: Matthew Nicholson (mnicholson) 2009-11-02 11:17:26.000-0600

ResetCDR will not increment the sequence number.  ForkCDR will though.  In the case of ForkCDR, some of the records will be the same as the one written to disk.
By: nick_lewis (nick_lewis) 2009-11-02 11:39:24.000-0600

For call monitoring I need the ability to start a new CDR (with new sequence) at any time of my choosing. This is not just for when the parties to a call change (e.g. transfers) but also when a party starts or stops monitoring a call so that the CDR reflects the actual duration of the recording. I believe that ResetCDR(w) is the correct way to accomplish this in the dialplan so I think ResetCDR(w) should change the sequence number.

The sequence number size looks a bit small now that it is global rather than per channel or per call/linkedid. Would a uuid be better if there are asterisk restarts?
By: Matthew Nicholson (mnicholson) 2009-11-02 11:53:24.000-0600

I will modify the patch so that ResetCDR(w) can be used in the manner you are trying to use it.
By: Matthew Nicholson (mnicholson) 2009-11-02 12:48:09.000-0600

Test the patch I just uploaded (cdr-sequence8.diff).  It should work in the manner you expect.
By: nick_lewis (nick_lewis) 2009-11-03 04:24:49.000-0600

Thanks

It looks good. I will give it a go once I have a system with trunk software up and running

Review
1. Regarding
+ * A 'sequence' field has been added to CDRs which can be combined with
+   linkedid or uniqueid and used as a unique identifier with for a CDR.

(i) There is a typo "with for"

(ii) Since the sequence is now global should this comment instead become
+ * A 'sequence' field has been added to CDRs which can be
+   used as a unique identifier for a CDR.


2. Regarding ast_cdr_dup_unique and ast_cdr_dup_unique2 functions:
These functions do not seem to add anything over exposing cdr_seq_inc and seem to make the code less easy to understand.

Suggest
- if (!(newcdr = ast_cdr_dup(cdr)))
+ if (!(newcdr = ast_cdr_dup_unique(cdr)))
return;
Become
- if (!(newcdr = ast_cdr_dup(cdr)))
+ if (!(newcdr = ast_cdr_dup(cdr))) {
+ cdr_seq_inc(newcdr);
return;
+ }

Suggest
- if ((duplicate = ast_cdr_dup(cdr))) {
+ if ((duplicate = ast_cdr_dup_unique2(cdr))) {
Become
if ((duplicate = ast_cdr_dup(cdr))) {
+ cdr_seq_inc(cdr);

Suggest
- bridge_cdr = ast_cdr_dup(chan_cdr);
+ bridge_cdr = ast_cdr_dup_unique2(chan_cdr);
Become
bridge_cdr = ast_cdr_dup(chan_cdr);
+ cdr_seq_inc(chan_cdr);

Suggest deleting ast_cdr_dup_unique and ast_cdr_dup_unique2 functions

3. Regarding features.c
What happens to the old chan_cdr once the bridge_cdr is created. Is it posted? If so should it instead have the original sequence number and bridge_cdr have the new one?
By: Matthew Nicholson (mnicholson) 2009-11-03 09:18:25.000-0600

1. I will fix the typo there.  But the sequence field cannot be used by it self to uniquely identify a CDR.  The sequence field is reset each time asterisk is restarted.  It must be combined with the uniqueid or linkedid field to uniquely identify a CDR.

2. It is true that these functions do not do very much, but I wanted to abstract away the details of sequence number management.  This includes how sequence numbers are generated (cdr_seq_inc) and when they are generated (ast_cdr_init, ast_cdr_dup_*).  I don't want developers working with CDR records to have to think about whether to increment the sequence number or not, I think it is easier to think about whether this CDR should be unique or not.

3. The bridge_cdr is copied from the chan->cdr.  From there several things can happen. In most cases, bridge_cdr is written to disk, and chan->cdr is eventually discarded (along with peer->cdr).  If a transfer occurs, the bridge_cdr is supposed to be written and the peer->cdr or chan->cdr reset for the next leg of the call, but I belive this code is buggy.

With the sequence number patch, the bridge_cdr gets the chan->cdr's sequence number and chan->cdr's sequence number is incremented.  In theory, this should cause the next leg of the call to have a new sequence number.
By: nick_lewis (nick_lewis) 2009-11-03 10:08:49.000-0600

1. Yes I see linkedid+sequence it is then (I still have a preference for a call/linkedid structure to hold a sequence per call rather than a global sequence shared by all calls but I accept that this adds complexity for little gain)

2. Ok but could the function names be a bit more informative.
ast_cdr_dup_unique_swap() may be better than ast_cdr_dup_unique2()

3. At the moment my dialplan does a Reset(w) if it sees the BLINDTRANSFER flag but it sounds as if this may not be necessary if the old cdr is posted and the new cdr produced as part of a transfer process. Does this also apply to channel tech specific transfers?
By: Matthew Nicholson (mnicholson) 2009-11-03 10:21:01.000-0600

2. I like that idea.  That is the name I was initially going to use for that function.  I will make this change.

3. I am not 100% sure how transfers are handled in all cases.  Asterisk has different code for '#' transfers, channel based transfers, and manager initiated transfers.  Additionally, this code interacts with the bridging code in different ways in different cases (e.g. the channel based CDR is not always reset).  Ideally, this code all needs to be audited and restructured to have uniform behavior where possible.

In the specific case you cite, you should not need to do ResetCDR(w) as the bridge CDR should be written for that call.  You may want to do ResetCDR() without the 'w' flag.  As I said before, this code my have bugs, so I recommend you test these scenarios yourself.  Please report any bugs you find.
By: Digium Subversion (svnbot) 2009-11-03 15:26:33.000-0600

Repository: asterisk
Revision: 227435

U   trunk/CHANGES
U   trunk/apps/app_forkcdr.c
U   trunk/configs/cdr_custom.conf.sample
U   trunk/funcs/func_cdr.c
U   trunk/include/asterisk/cdr.h
U   trunk/main/cdr.c
U   trunk/main/features.c

------------------------------------------------------------------------
r227435 | mnicholson | 2009-11-03 15:26:33 -0600 (Tue, 03 Nov 2009) | 8 lines

This patch adds a sequence field to CDRs that can be combined with the linkedid or uniqueid field to uniquely identify a CDR.

(closes issue ASTERISK-14189)
Reported by: Nick_Lewis
Patches:
     cdr-sequence10.diff uploaded by mnicholson (license 96)
Tested by: mnicholson

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=227435