[asterisk-dev] [Code Review]: Add AMI event documentation

Matt Jordan reviewboard at asterisk.org
Wed Jun 6 21:18:58 CDT 2012 


> On June 6, 2012, 7:05 p.m., Paul Belanger wrote:
> > Let me be the first to say, THANK YOU! This is an awesome patch for end users.  I'll be scheduling some time tomorrow to test.  A few comments in line, a side from that I am not a fan of having the xmldoc all over the source code, I'd prefer to see it at the top with the other documentation.  I understand it adds new functionality, but that just my opinion.
> > 
> > Thanks again, great work.

Thanks!

I'll answer the 'why is the documentation in the code' question below.


> On June 6, 2012, 7:05 p.m., Paul Belanger wrote:
> > ./trunk/Makefile, lines 318-324
> > <https://reviewboard.asterisk.org/r/1967/diff/1/?file=28570#file28570line318>
> >
> >     Why even have 'make full' and not just do it by default?
> >     
> >     Seems weird to me to have 2 different switches for documentation.
> >     
> >     Also, wouldn't 'make docs' be appropriate?

Two reasons:

1. The new documentation scripts are slower (you'll notice that when you test it).
2. They require python, which shouldn't be a prerequisite for 'normal' builds.

I went with 'make full' because 'make' still builds documentation.  I'd be fine with other suggestions.


> On June 6, 2012, 7:05 p.m., Paul Belanger wrote:
> > ./trunk/build_tools/make_version, lines 59-64
> > <https://reviewboard.asterisk.org/r/1967/diff/1/?file=28580#file28580line59>
> >
> >     bad merge?

Yup.  I'll remove it in the next diff.


> On June 6, 2012, 7:05 p.m., Paul Belanger wrote:
> > ./trunk/build_tools/post_process_documentation.py, lines 71-77
> > <https://reviewboard.asterisk.org/r/1967/diff/1/?file=28581#file28581line71>
> >
> >     optparse is deprecated in 2.7, we should change this to argparse if possible.
> >     
> >     http://docs.python.org/library/optparse.html

Yeah, I thought about that, and prefer its replacement argparse.  Unfortunately, argparse isn't available until version 2.7, which would probably mean that several distros that are still in heavy use wouldn't be able to use this functionality.  Hence, optparse.


> On June 6, 2012, 7:05 p.m., Paul Belanger wrote:
> > ./trunk/apps/app_chanspy.c, lines 543-550
> > <https://reviewboard.asterisk.org/r/1967/diff/1/?file=28571#file28571line543>
> >
> >     Why managerEventInstance and not just managerEvent? Curious.

This gets into the funkiness that are manager events.  Events can occur that are the same type, e.g., PeerStatus, but have different sets of parameters (think IAX2 peers versus SIP peers).  They can be raised in multiple implementation files.  At the same time, they often share common sets of parameters and other information.

The XML schema in the DTD shows this relationship.  A manager event can have multiple instances, where each instance represents a place in the code (or, at least, a logical distinct 'instance' of an event) that the event is raised.  If you look at a produced XML fragment, it would look something like the following:

<managerEvent name="foo">
  <managerEventInstance class="EVENT_CLASS_BLAH">
     <syntax>
       <parameter name="A"/>
       <parameter name="B"/>
     </syntax>
  </managerEventInstace>
  <managerEventInstance class="EVENT_CLASS_BLAH">
     <syntax>
       <parameter name="A"/>
       <parameter name="C"/>
     </syntax>
  </managerEventInstance>
</managerEvent>

The original design was to have users document all of this themselves at the top of the implementation file - but then that begged the question, which file?  If instance 1 (with parameters A and B) is in chan_sip, and instance 2 (with parameters A and C) is in chan_iax2 - where does the user document it?  How does it get combined?

At the point it became clear that post-processing was going to be needed, you can start to ask other questions.
1. Why does the user need to type <managerEvent>, when they know the name is "foo" and they want it to be combined with all other instances of "foo"?
2. Why document parameter "A" twice?  Why not document it once - anywhere - and document B and C where they are used?
3. Why document B or C at all, if its painfully obvious what they are?
4. If the parameters can be inferred from the methods that raise the events, why type them out unless they need explanation?

I figured no one had documented manager events before because it is, quite frankly, a pain.  They're all over the place, its repetitive, and there's no system of 'ownership', like you have with applications/functions/commands, that register themselves in a particular implementation file.  There are other concerns as well, but I wanted it to be as easy as possible to get *any* documentation for an event, even if its just a listing of the available fields.


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1967/#review6396
-----------------------------------------------------------


On June 6, 2012, 6:20 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1967/
> -----------------------------------------------------------
> 
> (Updated June 6, 2012, 6:20 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and wdoekes.
> 
> 
> Summary
> -------
> 
> This patch begins to add AMI event documentation to Asterisk.  This patch includes the following:
> 
> * Pulling AMI event XML documentation out of the Asterisk source and into the XML documentation produced by the build tools
> * Validating the XML when -dev-mode is enabled
> * Reading/parsing the XML documentation when Asterisk starts
> * Two new CLI commands: 'manager show events' and 'manager show event [event_name]'
> * Event documentation for the core applications
> 
> So... AMI events are a bit trickier then other things we've attempted to document in the past.  There were a number of factors to consider when approaching this:
> 1. There are a lot of AMI events, and they are located throughout the code base
> 2. AMI events can occur multiple times with different parameters, but still be the same 'event' type
> 3. AMI events can occur multiple times, in different implementation files
> 4. There is a significant amount of repeated information between events
> 5. Much of the information that a person cares about (AMI event keys) are - more often then not - easily obtained from a method call
> 
> To that end, documentation for AMI events is treated a bit differently then other items.  There are a few major shifts in how the documentation is extracted:
> 1. The previous documentation extraction used an awk script that pulled the documentation from a single comment block (delineated by /*** DOCUMENTATION ... ***/).  That still exists; however, if you want AMI event documentation, you have to use 'make full'.  This calls a different documentation extraction script, written in python.  That script lets us do a few things that the awk script did not:
>  * Documentation can now exist anywhere in the source file
>  * The documentation for manager events can be co-located with the method calls that raise the event.  If the method call is recognized, event parameters are automatically read from the method call and populated in the documentation
>  * Documentation is combined where appropriate.  Thus, if the same event is raised in multiple places, the parameters only need to be defined once.
> 2. Previously, documentation was registered with the application/function/command that it was associated with.  Events are never registered with any controller; requiring them to do so would incur a run-time performance penalty for little gain.  Hence, the xmldoc core now allow documentation for an entire source type to be requested at run-time.  When requested, the documentation is built from the XML read in.
> 
> Example manager events:
> 
> Event: ChanSpyStart
> [Synopsis]
> Raised when a channel has started spying on another channel.
> 
> [Syntax]
> Event: ChanSpyStart
> SpyerChannel: <value>
> SpyeeChannel: <value>
> 
> [See Also]
> ChanSpy(), ExtenSpy(), ChanSpyStop
> 
> -----------------------
> 
> Event: Dial
> [Synopsis]
> Raised when a dial action has started.
> 
> [Syntax]
> Event: Dial
> SubEvent: <value>
> Channel: <value>
> Destination: <value>
> CallerIDNum: <value>
> CallerIDName: <value>
> ConnectedLineNum: <value>
> ConnectedLineName: <value>
> UniqueID: <value>
> DestUniqueID: <value>
> Dialstring: <value>
> 
> [Arguments]
> SubEvent
>     Begin
>     End
>     A sub event type, specifying whether or not the dial action has begun
>     or ended.
> 
> [Synopsis]
> Raised when a dial action has ended.
> 
> [Syntax]
> Event: Dial
> DialStatus: <value>
> SubEvent: <value>
> Channel: <value>
> UniqueID: <value>
> 
> [Arguments]
> DialStatus
>     The value of the ${DIALSTATUS} channel variable.
> SubEvent
>     Begin
>     End
>     A sub event type, specifying whether or not the dial action has begun
>     or ended.
> 
> ---------------------------------
> 
> Event: QueueMemberStatus
> [Synopsis]
> Raised when a Queue member's status has changed.
> 
> [Syntax]
> Event: QueueMemberStatus
> Queue: <value>
> Location: <value>
> MemberName: <value>
> StateInterface: <value>
> Membership: <value>
> Penalty: <value>
> CallsTaken: <value>
> LastCall: <value>
> Status: <value>
> Paused: <value>
> 
> [Arguments]
> Queue
>     The name of the queue.
> Location
>     The queue member's channel technology or location.
> MemberName
>     The name of the queue member.
> StateInterface
>     Channel technology or location from which to read device state
>     changes.
> Membership
>     dynamic
>     realtime
>     static
> Penalty
>     The penalty associated with the queue member.
> CallsTaken
>     The number of calls this queue member has serviced.
> LastCall
>     The time this member last took call, expressed in seconds since 00:00,
>     Jan 1, 1970 UTC.
> Status
>     The status of the queue member.  This will be a device state value.
> Paused
>     0
>     1
> 
> 
> Diffs
> -----
> 
>   ./trunk/apps/app_chanspy.c 367982 
>   ./trunk/apps/app_confbridge.c 367982 
>   ./trunk/apps/app_dial.c 367982 
>   ./trunk/apps/app_meetme.c 367982 
>   ./trunk/apps/app_queue.c 367982 
>   ./trunk/apps/app_stack.c 367982 
>   ./trunk/apps/app_userevent.c 367982 
>   ./trunk/apps/app_voicemail.c 367982 
>   ./trunk/build_tools/get_documentation.py PRE-CREATION 
>   ./trunk/build_tools/make_version 367982 
>   ./trunk/build_tools/post_process_documentation.py PRE-CREATION 
>   ./configure UNKNOWN 
>   ./trunk/Makefile 367982 
>   ./trunk/configure.ac 367982 
>   ./trunk/doc/appdocsxml.dtd 367982 
>   ./trunk/include/asterisk/xmldoc.h 367982 
>   ./trunk/main/manager.c 367982 
>   ./trunk/main/xmldoc.c 367982 
>   ./trunk/makeopts.in 367982 
> 
> Diff: https://reviewboard.asterisk.org/r/1967/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120607/c8f6aa27/attachment-0001.htm>

More information about the asterisk-dev mailing list