[Home]

Summary:ASTERISK-07968: [patch] log jitter buffer stats regularly for post-mortems, route qualification etc etc
Reporter:Steve Davies . (stevedavies)Labels:
Date Opened:2006-10-20 04:21:27Date Closed:2008-02-14 18:23:20.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_iax2
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) jblogging-1.2.patch
( 1) jblogging-trunk_wmgrevent.patch
( 2) jblogging-trunk.patch
( 3) updated_jbloggin-trunk_mgrevent.patch
Description:This patch causes chan_iax2 to log a summary line into the verbose log every time an active iax2 call receives a PONG back from the other side (which in practice means every 20 seconds)

These log entries have been very very useful to us to post-mortem quality issues and to analyse so we can take routes out of service when they are not performing.



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


I've attached patches for both trunk and the 1.2 branch.

Trunk is against svn rev 45752,
branch-1.2 is against 45691.
Comments:By: damin (damin) 2006-11-02 07:07:23.000-0600

This looks like a really, really useful patch. The only suggestion that I would make is that the option should be a configurable option. I.E. it would be nice to be able to turn the feature on in the iax.conf file.

For example, if there was an option called;

; Enable logging of IAX2 Jitterbuffer statistics. Defaults to no
logjitterbuffer = no
By: Jared Smith (jsmith) 2006-11-02 13:31:38.000-0600

Interesting... John Todd and I were discussing something similar at AstriCon, but in a more generic sense.  What we'd like to see written is CQDR -- Call Quality Detail Records.  Think of them as sitting side-by-side the CDR records, but showing information about the Call Quality.  

In the case of IAX, we get all kinds of useful information, which we don't currently capture (and your patch logs to the verbose log).  In the case of SIP, we have RTCP reports (as well as call quality reports in the BYE messages of some endpoints).  

Wouldn't it be cool to see all this information logged to a CQDR table?
By: damin (damin) 2006-11-02 22:08:54.000-0600

That would be a fantastic idea!
By: Steve Davies . (stevedavies) 2006-11-10 08:19:35.000-0600

So where to for this patch?

I can easily add the configuration option, if you really want it (Asterisk already has so many, and these stats are only seen at VERBOSE level).

I'd like to see that patch go in as is or with that change.

We can then improve from there with a separate CQDR log if desired.

Steve
By: damin (damin) 2006-11-10 14:42:31.000-0600

Here is my opinion. I do not have any specific authority to tell you to do this, or that it will get accepted as a patch. However, I would suggest the following:

1. Add the option to allow this to be a configurable option in the iax.conf file.

2. Add an option to send this information via a Manager event so that a monitoring application could monitor and report in near-real time call-quality assessment for routes.
By: Steve Davies . (stevedavies) 2006-11-23 09:43:08.000-0600

> 1. Add the option to allow this to be a configurable option in the iax.conf file

Really?  We already have many many lines logged at verbose level.  We are adding one line every 20 seconds per active iax2 call containing useful info in a compact form and you feel it should be optional, with the result that the config gets further cluttered with yet another config option of slightly mysterious purpose.

I do think you should offer some rationale for why someone might want this turned off?

Do like the idea of pushing them out the manager port or similar, though.

Steve
By: damin (damin) 2006-11-23 10:36:51.000-0600

>  I do think you should offer some rationale for why someone might want this turned off?

Sure.

This may be useful information during debugging sessions and/or for spot-checking, but why waste the CPU cycles on high volume production system in the case that you do not need the information? Yes, you can adjust the verbose levels on the CLI and/or not run the CLI at all.

Also, in the case that an issue arose w/ the code causing a crash, a simple config change to disable it is a nice option, especially if you need the debugging information at that level from other areas of the system.

I'm a fan of being prepared and providing the system admin control of specific debugging messages that can be modified on the fly w/out recompiling, to provide customized levels of control.
By: Steve Davies . (stevedavies) 2006-11-24 01:31:10.000-0600

OK,

Let me advance my reasoning why unnecessary config options should be avoided:

Making everything that's new optional on flags in the config files clutters up the config files, bloats the config structures and bloats the code.  And bloats the support lists when people post about them when they are confused.  Asterisk is already very complicated to operate and administer. why make it worse?

What happened to simplicity, beauty, brevity as the goal of code review?

Community projects have to be oh-so-careful - each time we make a choice for an "extra config option", or the smaller in place patch rather than refactoring the code, then the overall code quality goes down a bit...  

If anyone can see a problem in the code then please please please point it out.  I don't want to be embarrassed by a stupid bug.  But you are already running a fair amount of patches that came from me for tricky parts of the code base.  And your Asterisk crashes less, not more, because of my efforts to find and fix races and other issues.  Look through my contributions here on the tracker.

Here we have a function that is called once per 20 seconds per active IAX2 call - 10 times per second for 200 concurrent IAX2 calls.  It merely printfs a bunch of data that is already stored in an internal structure.

Optimization is about putting effort in in the critical code paths.  Let me assure you that this will not be a critical code path.  This will not use more than 0.001% of total CPU in a typical high-load system.  Code executing 10 times per second (for 200 IAX calls) is certainly not going to rank against code executing 1,600,000 times per second (sample-by-sample loops in the codecs, echo canceller etc that must also run for those 200 IAX2 calls)

Steve
By: Jon Creasy (johann8384) 2006-12-28 12:25:16.000-0600

We should standardize these events with the events in ASTERISK-8375 as much as possible.


^-- nevermind, I thought these were manager events not verbose logs, I had that on the brain....I would like to update the patch though to add manager events for the same information.


By: Jon Creasy (johann8384) 2006-12-28 13:52:40.000-0600

I added a manager event with the same information and the new manager privelege "reporting" which is being discussed elsewhere.
By: Jon Creasy (johann8384) 2007-08-21 20:58:24

I uploaded a new patch against the current trunk. I did the sign an agreement thing and I'm waiting for legal to approve it before the download shows up.
By: Michiel van Baak (mvanbaak) 2008-02-01 17:19:09.000-0600

Indentation is really off here
By: Digium Subversion (svnbot) 2008-02-14 12:36:36.000-0600

Repository: asterisk
Revision: 103677

U   trunk/channels/chan_iax2.c

------------------------------------------------------------------------
r103677 | qwell | 2008-02-14 12:36:34 -0600 (Thu, 14 Feb 2008) | 11 lines

Add periodic jitter stats to CLI and manager.

(closes issue ASTERISK-7968)
Reported by: stevedavies
Patches:
     jblogging-trunk.patch uploaded by stevedavies
     jblogging-trunk_wmgrevent.patch uploaded by johann8384
     updated_jbloggin-trunk_mgrevent.patch uploaded by johann8384 (license 190)
     (with additional changes by me)
Tested by: stevedavies, johann8384

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

http://svn.digium.com/view/asterisk?view=rev&revision=103677
By: Digium Subversion (svnbot) 2008-02-14 18:23:20.000-0600

Repository: asterisk
Revision: 103708

_U  team/murf/bug11210/
U   team/murf/bug11210/UPGRADE.txt
U   team/murf/bug11210/apps/app_externalivr.c
U   team/murf/bug11210/apps/app_queue.c
U   team/murf/bug11210/apps/app_voicemail.c
U   team/murf/bug11210/channels/chan_iax2.c
U   team/murf/bug11210/configs/queues.conf.sample
U   team/murf/bug11210/configure
U   team/murf/bug11210/configure.ac
U   team/murf/bug11210/doc/tex/imapstorage.tex
U   team/murf/bug11210/funcs/func_cdr.c
U   team/murf/bug11210/include/asterisk/autoconfig.h.in
U   team/murf/bug11210/res/res_agi.c
U   team/murf/bug11210/res/res_musiconhold.c

------------------------------------------------------------------------
r103708 | murf | 2008-02-14 18:23:17 -0600 (Thu, 14 Feb 2008) | 187 lines

Merged revisions 103658,103662,103668,103677,103682,103685,103687,103689,103691,103694,103699-103700,103702,103704-103705 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r103658 | mmichelson | 2008-02-13 08:47:25 -0700 (Wed, 13 Feb 2008) | 10 lines

1. Deprecate SetMusicOnHold and WaitMusicOnHold.
2. Add a duration parameter to MusicOnHold

(closes issue ASTERISK-11358)
Reported by: dimas
Patches:
     v2-moh.patch uploaded by dimas (license 88)
 Tested by: dimas


................
r103662 | jpeeler | 2008-02-13 14:04:31 -0700 (Wed, 13 Feb 2008) | 6 lines

(closes issue ASTERISK-11286)
Reported by: ctooley
Patches:
     additional_eivr_commands.patch uploaded by ctooley (license 136)
Tested by: ctooley

................
r103668 | oej | 2008-02-14 03:19:09 -0700 (Thu, 14 Feb 2008) | 2 lines

Formatting fixes

................
r103677 | qwell | 2008-02-14 11:39:51 -0700 (Thu, 14 Feb 2008) | 11 lines

Add periodic jitter stats to CLI and manager.

(closes issue ASTERISK-7968)
Reported by: stevedavies
Patches:
     jblogging-trunk.patch uploaded by stevedavies
     jblogging-trunk_wmgrevent.patch uploaded by johann8384
     updated_jbloggin-trunk_mgrevent.patch uploaded by johann8384 (license 190)
     (with additional changes by me)
Tested by: stevedavies, johann8384

................
r103682 | jpeeler | 2008-02-14 12:47:39 -0700 (Thu, 14 Feb 2008) | 1 line

a few syntax changes and safer code
................
r103685 | qwell | 2008-02-14 12:52:21 -0700 (Thu, 14 Feb 2008) | 13 lines

Merged revisions 103683 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r103683 | qwell | 2008-02-14 13:51:10 -0600 (Thu, 14 Feb 2008) | 5 lines

Document the 'l' option to the CDR() function.
(Thanks voipgate for pointing out the option, and Leif for providing text for it.)

Closes issue ASTERISK-11166.

........

................
r103687 | mmichelson | 2008-02-14 13:46:00 -0700 (Thu, 14 Feb 2008) | 10 lines

Change the queue holdtime announcement to happen at any interval (not just greater than two minutes). Remove
the saying of less-than for holdtime announcements since it can lead to awkward holdtime announcements. Using
'1' as a queue-round-seconds value is no longer valid.

(closes issue ASTERISK-9451)
Reported by: caio1982
Patches:
     queue_announce5.diff uploaded by caio1982 (license 22)
 Tested by: caio1982, putnopvut

................
r103689 | mmichelson | 2008-02-14 13:58:30 -0700 (Thu, 14 Feb 2008) | 17 lines

Merged revisions 103688 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r103688 | mmichelson | 2008-02-14 14:55:48 -0600 (Thu, 14 Feb 2008) | 9 lines

Fix the new message count if delete=yes when using IMAP storage.

(closes issue ASTERISK-10917)
Reported by: jaroth
Patches:
     deleteflag_v2.patch uploaded by jaroth (license 50)
 Tested by: jaroth


........

................
r103691 | mmichelson | 2008-02-14 14:04:37 -0700 (Thu, 14 Feb 2008) | 11 lines

Merged revisions 103690 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r103690 | mmichelson | 2008-02-14 15:03:02 -0600 (Thu, 14 Feb 2008) | 3 lines

Fix build for non-IMAP builds


........

................
r103694 | qwell | 2008-02-14 14:21:53 -0700 (Thu, 14 Feb 2008) | 8 lines

Modify ldap autoconf function, so that an incorrect ldap library is not found on Solaris (it is incompatible).
Also removes second check for awk, which causes Solaris to find an incompatible version of awk.

(closes issue ASTERISK-11290)
Reported by: snuffy
Patches:
     bug-11829.diff uploaded by snuffy (license 35)

................
r103699 | mmichelson | 2008-02-14 16:35:21 -0700 (Thu, 14 Feb 2008) | 20 lines

Blocked revisions 103698 via svnmerge

........
r103698 | mmichelson | 2008-02-14 17:30:17 -0600 (Thu, 14 Feb 2008) | 13 lines

Change to the configure logic regarding IMAP. Prior to this commit, if you wished to configure
Asterisk with IMAP support, you would use the --with-imap configure switch in one of the following
two ways:
--with-imap=/some/directory would look in the directory specified for a UW IMAP source installation
--with-imap would assume that you had imap-2004g installed in .. relative to the Asterisk source

With this set of changes the two above options still work the same, but there are two new behaviors, too.
--with-imap=system will assume that you have -libc-client.so where you store your shared objects and will
           attempt to find c-client headers in your include path either in the imap or c-client directory.
If either of the two original methods of specifying the imap option should fail, then the check for --with-imap
=system will be performed in addition. It is only after this "system" check that failure can happen.


........

................
r103700 | mmichelson | 2008-02-14 16:39:47 -0700 (Thu, 14 Feb 2008) | 5 lines

See commit message for svn revision 103698. This behavior is the same as what is described
there. The difference is that trunk already had the --with-imap=system option, but it only
checked the include path for headers in the imap directory and not also the c-client directory.


................
r103702 | mmichelson | 2008-02-14 16:48:12 -0700 (Thu, 14 Feb 2008) | 10 lines

Blocked revisions 103701 via svnmerge

........
r103701 | mmichelson | 2008-02-14 17:44:17 -0600 (Thu, 14 Feb 2008) | 3 lines

Update documentation regarding configuration of IMAP


........

................
r103704 | mmichelson | 2008-02-14 16:49:54 -0700 (Thu, 14 Feb 2008) | 10 lines

Blocked revisions 103703 via svnmerge

........
r103703 | mmichelson | 2008-02-14 17:49:24 -0600 (Thu, 14 Feb 2008) | 3 lines

Make a small clarification in the documentation


........

................
r103705 | mmichelson | 2008-02-14 16:51:49 -0700 (Thu, 14 Feb 2008) | 3 lines

Trunk version of 1.4's imap documentation updates


................

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

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