Re: Proposal for speeding up TicketAcls

Hi,

On 12/12/2013 09:18 AM, Martin Gruner wrote:
> Hi Moritz,
>
> this sounds great to me.

and it turns out it's not too hard to implement either :-)
See https://github.com/OTRS/otrs/pull/180

> Am 10.12.13 13:58, schrieb Moritz Lenz:
>> Hi all,
>>
>> recently I found out that although we use only very few ACLs, the ACL
>> mechanism slows down our OTRS installation. Part of the problem is that
>> our CustomerUser-backend talks to an external data source, which is
>> slow. But the problem is more general: Lots of data is assembled in
>> Kernel::System::Ticket->TicketACL, which isn't actually used later on.
>> Since ACLs are also checked for ticket lists (like in AgentTicketQueue
>> or AgentTicketSearch), this can be a significant overhead.
>>
>> For normal ACLs, we can easily find out which data they actually use.
>> For ACL modules, I propose we add extra configuration.
>>
>> Before, the configuration of an ACL module looks like this in
>> Kernel::Config:
>>
>>     $Self->{'Ticket::Acl::Module'}->{'TheAclModulename'} = {
>>         Module => 'Kernel::System::Ticket::Acl::TheAclModule',
>>     };
>>
>> If we extend that to something like this:
>>
>>     $Self->{'Ticket::Acl::Module'}->{'TheAclModulename'} = {
>>         Module => 'Kernel::System::Ticket::Acl::TheAclModule',
>>         Checks => ['Owner', 'Queue', 'SLA', 'Ticket'],
>>         ReturnType => 'Ticket',
>>         ReturnSubType => ['State', 'Service'],
>>     };
>>
>> we can apply two optimizations:
>>
>> 1) Only invoke an Acl Module if the ReturnType and ReturnSubType
>> overlap with the TicketAcl params ReturnType and ReturnSubType
>> 2) Only gather data for the requested Checks elements. If for example no
>> Acl and no ACL module depends on the Service or the CustomerUser, than
>> those pieces of infomation don't need to retrieved at all.
>
> Yes. This can be implemented backwards compatible, i. e. if the
> configuration is not present, always run the ACL.

That's how I've done it in 
https://github.com/moritz/otrs/commit/e1ccd3f5d8b625580e52c41d21d8846062f9f32f

>> As a further note, most information seems to show up twice in the
>> %Checks argument to the Acl module, for example both as $Checks{Owner}
>> and $Checks{Ticket}{Owner}. To get the desired speedup, a module which
>> only lists the check 'Ticket' would get whatever data TicketGet returns,
>> not the additional data that is currently added in method TicketAcl.
>
> I'm not sure about this one. Let's hear what Carlos Rodriguez  has to
> say about it.

I have mitigated possible fallout from this approach by looking a bit 
deeper into the ACL. If for example an ACL has

Properties => {
    Ticket => {
       Queue => ...
    }
}

then both the Ticket and the Queue data is fetched. Which makes all the 
available ACL unit tests pass.

>> P.S. for the sake of completeness I've gone through Kernel::System::Ticket,
>> and looked up the possible values for Checks, ReturnType and ReturnSubType.
>> Here they are:
>>
>> Checks:
>>     CustomerUser
>>     DynamicField
>>     Frontend
>>     Owner
>>     Priority
>>     Process
>>     Queue
>>     Responsible
>>     Service
>>     SLA
>>     State
>>     Ticket
>>     Type
>>
>> ReturnType:
>>     Action
>>     Ticket
>>
>> ReturnSubType:
>>     Type
>>     Queue
>>     State
>>     Service
>>     SLA
>>     Priority
>>
>
> Nice. Please feel free to add this to the POD, and also to improve the
> existing code where you see fit.

I'll update the documentation a bit.

Cheers,
Moritz
_______________________________________________
OTRS mailing list: dev - Webpage: http://otrs.org/
Archive: http://lists.otrs.org/pipermail/dev
To unsubscribe: http://lists.otrs.org/cgi-bin/listinfo/dev