Support for ChanVariable(channel): MY_VAR=value

[johanwilfer]

In Asterisk manager.conf the channelvars option exists:

;
; Display certain channel variables every time a channel-oriented
; event is emitted:
;
;channelvars = var1,var2,var3

So if one would define channelvars = var1,var2,var3 you would get the following appended to all channel-oriented events:

ChanVariable(SIP/jw9933-00000062): var1=14
ChanVariable(SIP/jw9933-00000062): var2=value-of-var2
ChanVariable(SIP/jw9933-00000062): var3=value-of-var3

However, PAMI doesn't understand this syntax and you end up with only one key ChanVariable(SIP/jw9933-00000062) that has the value var3=value-of-var3 as var1 and var2 is overwritten.

So I have this (quite ugly I know) code to parse it for now:

    /**
     * @return array
     */
    protected function getChannelVariables(\PAMI\Message\Event\EventMessage $event)
    {
        $channel_vars = array();
        $channel_id = $event->getKey('Channel');

        // we can't get the variables if we doesn't have a channel
        if ($channel_id === null)
            return $channel_vars;

        $prefix = "ChanVariable(" . $channel_id . "):";
        $length = strlen($prefix);
        $rows = explode("\n", $event->getRawContent());
        foreach ($rows as $row)
            if (substr($row, 0, $length) == $prefix)
            {
                list($chanvar, $data) = explode('=', substr($row, $length));
                $channel_vars[trim($chanvar)] = trim($data);
            }
        return $channel_vars;
    }

There actually could be variables for different channels in some cases, but I don't use it right now so the above code doesn't support that.

I would like to have a more clean way of doing this. Some proposals:

• If a key exists more than once during parsing, put them in an array so we don't overwrite values
• Add a getChannelVariables($channel = null) helper to the base-class (EventMessage?) that parses the values and return them for the given channel. If no channel is specified then we merge the variables for all channels.
• Maybe also add a getChannelVariable($channel, $key) that only looks for one specific key.
• Another way?

I can implement this and make a PR if I this will be accepted as a contribution.

Thanks for a great library by the way!

[jacobkiers]

Bug-fixing PRs are always welcome :)

There actually could be variables for different channels in some cases, but I don't use it right now so the above code doesn't support that.

Which events for example? And if you contribute a PR for this, will that implementation support that use case?

Regarding your suggested implementation: first off, yes, you're right, this should be in EventMessage, if it is only in events.

Concerning the place where this parsing should happen, I am a bit divided.

On the one hand, in line with how PAMI now works, I think these variables should be parsed when the event is created. In that case, it should indeed be done in the constructor of EventMessage, which then needs to override the constructor of IncomingMessage.

On the other hand, especially in high-throughput environments, adding more parsing logic to the constructors will slow every client down (even if it is a little). It doesn't matter when an Asterisk machine isn't too busy, but when you regularly get a throughput of a few hundred events/sec it adds up.

In that case, parsing it the first time (and only the first time) that the proposed ::getChannelVariable() is called, would be great.

As an aside: having a convenience method to get these variables make things just a little easier to work with, so I would be in favour of that.

Whether or not that would be accepted is of course for @marcelog to decide.I cannot think of a reason why not, though.

[johanwilfer]

I don't remember the exact event, but think it was some event related to Dial or Bridge, as we then has two channels involved in the event. It's an edge-case, but yes - I intend on support that.

Given that you in all normal cases just want the variables I propose ::getChannelVariables($channels = null) and if channels isn't specified then we return the variables for all channels (normally just one).

How about the parsing of duplicated headers? Right now we overwrite them, should we put them in a array if we got more than one? This would affect backwards compatibility.

[marcelog]

Hi guys,

@johanwilfer Could you paste some examples? The more complex the better.

So far, I would be in favor of a PR that parses this in the constructor of EventMessage, as pointed out by @jacobkiers. We could parse it and store an associative array indexed by channel name, where the value is an associative array itself, a typical K/V, then the method getChannelVariables would be a getter instead of a helper, returning everything we got. The caller could then traverse that as needed.

This is just a first thought, though, let's see the examples (and/or PR) first!

Thanks!

[marcelog]

mmm I did a quick test, and I don't get the channel name between parenthesis:

Event: Hangup
Privilege: call,all
Channel: SIP/marcelog-00000000
ChannelState: 4
ChannelStateDesc: Ring
CallerIDNum: marcelog
CallerIDName: marcelog
ConnectedLineNum: <unknown>
ConnectedLineName: <unknown>
AccountCode:
Context: marcelog
Exten: 333
Priority: 1
Uniqueid: 1440157474.0
ChanVariable: var1=
ChanVariable: var2=
ChanVariable: var3=
Cause: 0
Cause-txt: Unknown

This is asterisk 12.4.0

[johanwilfer]

Asterisk 1.8

Event: Dial
Privilege: call,all
SubEvent: Begin
Channel: Local/0@pbx_dial_callroute_to_endpoint-00000008;2
Destination: SIP/jw1034-00000010
CallerIDNum: 1201
CallerIDName: <unknown>
ConnectedLineNum: strategy-sequential
ConnectedLineName: <unknown>
UniqueID: pbx-1439974866.33
DestUniqueID: pbx-1439974866.34
Dialstring: jw1034
ChanVariable(Local/0@pbx_dial_callroute_to_endpoint-00000008;2): var1=value
ChanVariable(Local/0@pbx_dial_callroute_to_endpoint-00000008;2): var2=value
ChanVariable(Local/0@pbx_dial_callroute_to_endpoint-00000008;2): var3=value
ChanVariable(SIP/jw1034-00000010): var1=value
ChanVariable(SIP/jw1034-00000010): var2=value
ChanVariable(SIP/jw1034-00000010): var3=value

[marcelog]

Ok, so we have variants, with and without the channel name, the patch in question should take both into account

[johanwilfer]

Asterisk 11 looks the same. So the format seems to have changed in Asterisk 12 as per issue #56 The Dial event is strange one here, maybe because of the local channel.

[johanwilfer]

I'll start working on something. Thanks for the feedback!

[marcelog]

Oh I think I have a small patch that will work.. let me show it to you guys first to know your thouhts, I just need to write a few tests

[marcelog]

@johanwilfer @jacobkiers any thoughts on this one? https://github.com/marcelog/PAMI/pull/86

[johanwilfer]

Wow! You are fast :-) Maybe we should add some tests, for example a more simple case with only one channel and asterisk 12 syntax. I'll try this and get back to you.

[marcelog]

Thank you @johanwilfer. If you can, please test it with your real use case scenario. I've added some more tests "just in case".

[johanwilfer]

Testing now.. One question - why the lowercasing of variable names and channel-names? Isn't it better to leave it as-is?

[marcelog]

Well, IMHO it's better to normalize it because even asterisk doesn't honor the case in variable names in different versions. Doing this allows one to avoid some subtle bugs, your code will never depend on the casing of the names (and it will be really weird that someone would want that vAriABLE and variable refer to 2 different variables).

[johanwilfer]

I've tested now, it works great. You are right about the lowercase - I change to lowercase in all the cases in the application now and it works the same.

I will run some more test after the weekend at the office, but it seems to work great.

Thank you @marcelog and @jacobkiers !

[marcelog]

Great! Thank you for the report and testing!

I'll merge on monday, feel free to come back and report any good or bad findings.

[johanwilfer]

I've run some more tests now, it works perfectly. Thank you for this!