[Home]

Summary:ASTERISK-20742: [patch] Log callers entering/leaving MOH.
Reporter:Steve Murphy (murf)Labels:
Date Opened:2012-11-26 18:06:54.000-0600Date Closed:2017-12-12 20:06:54.000-0600
Priority:TrivialRegression?
Status:Closed/CompleteComponents:Applications/app_queue
Versions:SVN 12 Frequency of
Occurrence
Related
Issues:
Environment:allAttachments:( 0) qlogholdtime_v2.patch
( 1) qlogholdtime_v3.patch
( 2) qlogholdtime.patch
( 3) qonhold.trunk.diff
( 4) qonholdv2.trunk.diff
Description:This patch will allow incoming calls to track the time they are put on hold (MOH). This does not count MOH played before an agent accepts the call.
Comments:By: Steve Murphy (murf) 2012-11-26 18:16:32.000-0600

This code introduces two new Queue Log events:

CALLERONHOLD
CALLEROFFHOLD

This is done by adding two channel vars after the call is connected in app_queue. Code in res_musiconhold will check for these vars and issue a queue log report at the proper moment(s).

This allows hold time to be measured for queued callers, a potentially useful metric in the call-center world.


By: Steve Murphy (murf) 2012-11-26 18:18:47.537-0600

I have tested this code on polycom phones, and it works. More testing is ALWAYS welcome!



By: Richard Mudgett (rmudgett) 2012-11-26 18:36:33.208-0600

This patch will not compile.  Since Asterisk 11 you cannot dereference an ast_channel pointer anymore.
chan->uniqueid
is now referenced as
ast_channel_uniqueid(chan)

By: Richard Mudgett (rmudgett) 2012-11-26 18:39:41.680-0600

I am also concerned about the spread of knowledge about app_queue.  Now res_musiconhold has knowledge about something for app_queue.

By: Steve Murphy (murf) 2012-11-27 22:58:51.742-0600

OOooops! My apologies. I *thought* I had this compiled on trunk; but as it turns out, I did not. So I took the time to fix, and then test this patch on trunk. Looks good.

By: Steve Murphy (murf) 2012-11-27 23:14:40.348-0600

Now, as to the spread of knowledge about app_queue. I was concerned about this myself. In my research, I discovered that
the queue_log function is defined in main/logger.c; queue_log is called from (besides app_queue.c) chan_agent.c; and
with this patch, also in res_musiconhold.c.

So, knowing that the added code depends on functionality provided in the core (main) of asterisk, and that app_queue
need not even be loaded, and no added dependencies are therefore created, I felt better.

I also pondered what it would take to keep the code purely inside app_queue, and it started getting messy with callbacks, and more files affected, that I gave up on the alternatives. Perhaps someone would like to spend the time?

For instance, it occurred to me that queue_log could be eradicated from the core entirely, in favor, say, of a CEL post processor generating it from the event stream. We'd have to be sure to generate all the queue events, and the post processor
would have to be written. I can guarantee that this would be a fair sized project, and yield little value over what exists already.


By: Richard Mudgett (rmudgett) 2012-11-28 10:54:42.083-0600

What chan_agent logs with the queue_log function is independent of the use of queues.  chan_agent simply logs when an agent logs in and out.

It would be better to redefine the res_musiconhold component roll to just log channels going into and out of MOH.  This would make res_musiconhold independent of queues for this purpose.  If queues don't already, you could log when a queue member answers.  Then you can do post processing like for CEL to determine the length of time a call is on hold while talking to a queue member.



By: Steve Murphy (murf) 2012-11-29 08:15:12.329-0600

Richard-- I'm not setting precedent here. It was set in chan_agent. You haven't explained why it is imperative that channels NOT be accessed to make queue logging throughout the core of Asterisk OK. It seems a rather arbitrary measure, and not even practical.

CEL exists because there was/is a huge problem with CDR's: you simply cannot store the dual-channel information that CDRs require on a single channel. You have to store it outside any channel. So, CEL gave us a way to store the channel events "offline". Trouble is, CEL can do the storage, but no-one (afaict) has finished the process and written a CEL->CDR converter/generator. I was going to, but economics got in the way. Right now, CEL is a cool thing, but almost useless until the second half is written. (there may be a few individuals doing this on their own out there somewhere, who knows).

CEL was never meant as a logging unification solution... but, sure, it could be. I'm not sold on the fact that unifying logging would be beneficial over what we have now. It certainly won't be more efficient. So until then, we have AMI log events to port 5038, we have CDR's being logged to databases/log files, CEL being logged to databases/log files, we have random events logged to console/files, and we have the queue_log.

Queue_log has been lucky to require very little in the way of events outside of app_queue. But there was never any guarantee that it someday would not. And here is one of those days. The need for hold information is out there, and this patch supports
it in a fairly minimal, simplistic way, without breaking any precedents or introducing any new dependencies. The need is for real-time reporting.

We can't simply try to force users to use other mechanisms, either. MOH is information is already published via AMI. But there are packages out there that don't use AMI, that do use queue_log.  And it would be horribly inefficient to have them open the AMI and monitor it for just HOH events. Asterisk can't handle very many AMI connections.

So, let's get practical. Anyone else out there want to throw in their opinion?





By: Richard Mudgett (rmudgett) 2012-11-29 11:17:12.933-0600

Yes you are setting precedent here for queue_log.  You are passing information to res_musiconhold from app_queue in the IN_QUEUE and IN_QUEUE_MEMBERNAME channel variables.  chan_agent does not get any information from another module to queue_log when agents log in and log out.

You have misunderstood my suggestion.  What I was trying to get across was to make res_musiconhold always queue_log when a channel starts and stops playing MOH.  This way res_musiconhold is still independent of app_queue because res_musiconhold does not get any information from app_queue to produce its logs.  Then _like_ CEL you can do post processing on queue_log to determine the length of time a caller was placed on hold.

By: Steve Murphy (murf) 2012-11-29 12:24:09.835-0600

Richard-- OK, I understand the request, and it is do-able, but I really don't understand why you are so against the passing of information from app_queue, and I'm afraid neither will the poor souls who must filter all the extraneous on/off-hold events from the queue log. Exactly what is the rationale for requiring them to filter out all the extra information?

By: Lorenzo Emilitri (lenz.loway) 2012-11-30 04:45:16.988-0600

I am not sure that writing all MOH events to queue_log would be correct, not because they would not be able to filter out, but because they belong there only if the call is queued. That's why some coupling is unavoidable.

On the other side, today's call-centers using Asterisk need this, because it is a major point that is missing and they were used to having this on legacy systems. Tracking MOH for calls strongly reflects the quality of service that they are offering, so they want to monitor this like they do for other queue parameters.


By: Leif Madsen (lmadsen) 2012-12-03 06:44:11.705-0600

Seems like this is a conversation that has amounted to something that should be taken to the asterisk-dev mailing list. These are the types of decisions that should be communicated openly, rather than a discussion in the bug tracker. Perhaps some other viewpoints and thoughts can help bring this issue to resolution. Thanks!

By: Rusty Newton (rnewton) 2012-12-06 17:35:35.222-0600

I'm going to suspend this until a resolution is reached. Please move the conversation to the asterisk-dev list.

Ping any bug marshal on the list or in #asterisk-bugs to re-open the ticket.



By: Steve Murphy (murf) 2013-01-11 17:30:38.919-0600

This new patch hopefully passes muster for the folks who would like to see the work of logging being done within the confines of app_queue via callbacks.

The goal was to use callbacks, and also, to use standard components to do so. Using ast_event seemed natural, as it is already being used in app_queue.

I went perhaps one step further than required, in that I saw that queue_log was being called from within chan_agent. Now, it isn't that unnatural that chan_agent is calling queue_log, after all it *is* a queue-related chunk of code! But, really, it seemed to me a little inconsistent, that such an app_queue specific action is being called from another module. So, it used the same event call-back mechanism to pull all queue logging back into app_queue. If this is seen as a negative thing, I can revert the chan_agent and app_queue code fairly easily to remove this.

Since the filtering is done in app_queue, you will see that I implemented a hash table for the channels that have been connected to agents. For queues with large memberships, and high call rates, this will be a definite performance advantage.


By: Steve Murphy (murf) 2013-01-11 18:07:05.922-0600

oh, one more thing. Testing!

I've tested this on trunk, and it works as well as the previous patch
for MOH events.

One thing I haven't tested: the chan_agent changes. Would a current user
of chan_agent like to give this a spin and report back on whether it
works the same as the normal code?


By: Richard Mudgett (rmudgett) 2013-01-11 22:13:51.593-0600

I like this approach much better and it even improves chan_agent.  Just looking at the patch diff, it could use some coding guidelines tweaks: curly braces, use of ast_free instead of free.

By: Steve Murphy (murf) 2013-01-12 16:04:34.538-0600

Fixed the curlies and changed any free() calls to ast_free(). I noted that not just the code in my patch needed updating. Therefore, a few extra changes are now in the patch.

By: Steve Murphy (murf) 2013-02-08 18:00:34.573-0600

Please find that I have attached:

qlogholdtime_v3.patch

I've noticed that there was a conflict with the trunk code; this is now fixed
and should apply without incident to newly updated trunk code.

There has been little activity on this issue; do I proceed to the reviewboard,
or...?



By: Richard Mudgett (rmudgett) 2013-02-08 18:10:36.782-0600

Yes it should go to reviewboard.

By: Corey Farrell (coreyfarrell) 2017-12-12 20:06:54.559-0600

Work was posted for review at https://reviewboard.asterisk.org/r/2417/ but findings were not addressed.

----

Suspended due to lack of activity. Please request a bug marshal in #asterisk-bugs on the IRC network irc.freenode.net to reopen the issue should you have the additional information requested. Further information on issue tracker usage can be found in the Asterisk Issue Guidlines \[1\].

\[1\] https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines