RE: [NeoStats-Devel] SeenServ queries

DeadNotBuried wrote:
> Mark Wrote:
Thanks for the run down on the vars.

> sef (seen extra found- i think) 

This is why I suggested better variable names or comments for the future :)

> > Why when one but not the other of these variables being set 
> result in 
> > a memcpy of the current seendata (os_memcpy( sdo, sd, 
> sizeof( SeenData ) ))?
> > The copy is never modified so why is it not possible to use the 
> > original pointer?
> 
> on a SEENNICK the senf[5][maxnick+3] aren't used at all, they 
> are only output if it is a SEEN, so there is no need to set 
> them to the nick found.
> the os_memcpy was to ensure the latest match found was always 
> available, as if it is from a SEEN it won't be pointing to 
> the same list entry, the copy was used to save having to 
> traverse the list again to find the first match on a SEEN , 
> it is also copied on the SEENNICK so the code below can be 
> entered just once instead of once for the SEEN and once for 
> the SEENNICK to use different variables.
> 
> this particular code was done before i started playing with 
> pointers properly (at least i hope i am getting them 
> right:)). it would have been better to just save the pointer 
> to refer to the original data. i'll change that later today. 

OK.

> although another change i've seen you commit will mean one 
> variable does need copying (ircops and above no can longer 
> see the real host/ip address when using the SEEN or SEENNICK 
> as a /msg SeenServ
> SEEN[NICK])

This was an accidental change so I have undone it.

> > How important is the max entries to seenserv? Currently, 
> every event 
> > does
> a
> > potentially very expensive list walk so a timer would be a 
> much better 
> > candidate since even running once every minute would avoid 
> potential 
> > multiple runs per second. Given the expiry setting is running at a 
> > resolution of day, the life timer at least should be on a day 
> > resolutuin timer rather than per event. Any objections to 
> me making such a change?
> 
> i was of the understanding that the list_count just retruned 
> a list variable set to the number of records in the list. 
> thats why i did the check each time a new record is added, so 
> that as one record was appended, and the list count went 
> above the limit, it would just remove the first list record. 
> thus keeping the count at the level set always.

Listcount does just return a variable, more or less. What I have changed in
this so far is to lookup the count once then process locally and stop
restarting the list parse by getting the next node before deleting the head.

At the moment every single event creates a new record which is an overhead
we should be able to reduce. 

> the expiry timer was added later after being suggested on the 
> list, so it was just added to the current check. my thoughts 
> were that on a large network, with things happening 
> constantly, only a small number of records from the start of 
> the list would be removed. but if done on a timer, depending 
> on the timer interval it could seem to hang services while 
> checking for expired records. due to comments made previously 
> by both yourself and Justin i thought it safer to go with 
> constant removal so that it was only the initial loading that 
> might seem to hang services.

It is not a loading overhead, it is a constant run time one. The while loop
is an OR so it will continue to delete records while there are too many OR
while there are old records. The check itself is made on every event so if I
join a network, join 20 channels, get kicked from 5, leave IRC thus parting
15 channels we will perform this check at least 42 times within a potential
time period of a few seconds just for one user. Extrapolate to a given
network and SeenServ is somewhat expensive to run.

Since the expiry time is on a resolution of day, we will be making a check
many times throughout the day and failing while a timer of say midnight
would expire an old entries with a single walkthrough matching the expiry
time resolution. It will have to do more work at one time, but the overhead
will be completely predicatable rather than the present system which could
create a number of stalls especially during high network activity.

For testing against the max count, it is probably better to do it only when
creating a list node. This means we can only ever be at the max before we
delete the oldest record if we reach the max to make room for a new record.
The expiry check can then be moved to a timer or something with an
additional check after loading the database during init since this could
potentially need the removal of many records. Combined with some
optimisation of the record management, we should be able to elimitate
potentisl stalls.

> > I have some concerns over the use of verbose in SeenServ. 
> Enabling it
> would
> > report pretty much every event a user generates while on IRC which 
> > introduces a potential flood of information it would generate. The 
> > information reported is normally the kind of information used for
> debugging
> > so it might be useful in an #if debug or dlog call rather 
> than part of 
> > normal run time operation as with similar reports in the 
> core and modules.
> > Any objections to me making such a change?
> 
> that is why the default is off :). I have no objections 
> personally, it was originally done that way so I could see 
> what was happening if a problem occurred with it. 

I thought it might have been that way.

Related to the verbose discussion, seenserv uses verbose to perform command
reporting. NeoStats noise was a criticism in 2.5.x so 3.0 includes a setting
which enables or disables command reporting in the core command processor.
With current SeenServ if verbose is on and neostats cmdreport is on, every
command use will be reported twice and in different formats. I suggest we
remove the current reporting within SeenServ to avoid this issue. 

In addition, SeenServ produces other messages via chanalert that should
probably within a verbose check. E.g. the channel setting announcements.

> as a side 
> effect though it does allow joins and parts for public 
> channels to be seen in the services channel when set, which I 
> have seen requested for connectserv before. 

I do not think SpyServ as a side effect is something we really want to
release. When I wrote my own SeenServ for NeoStats (never finished so never
released) I used a similar chanalert during development. This created a
great deal of concerns from other opers in the channel that such information
was being reported. It would be trivial for someone to code a module which
did this job specifically but I do not think we should distribute an
official module with such capabilities. The Unreal team are still unable to
shake the reputation of developing an intrusive user mode and such real time
reporting is too close to that realm for me.

Although it could be said that SeenServ is itself intrusive, without the
real time reporting, it offers little beyond the capabilities of all IRCds
anyway and is potentially open to users as well. As long as SeenServ
requires a specfic request for information rather than having it sent
regardless, it remains within the realm of acceptable.

> I've actually 
> been thinking about dlog calls, but deciding on what level to 
> use can be harder than deciding what should be logged in them 
> sometimes, so if there's is any guidelines to what sort of 
> messages at what level, life would be simpler :)

The only guideline really is to try and order by importance so that less
important information is not displayed at low levels of debug output. Since
generally only we see it, do not worry about it too much, just try not to
put everything in DEBUG1.

> I have thought about adding another option to enable lookup 
> by full NickName only, in that instance, the entire memory 
> list would be dropped if the option was enabled, and have it 
> work completely from the DB only. it would be an either/or 
> system, as it works now, or with no in memory list, if 
> changing between the 2 systems it would require a module 
> reload after changing the option. the only problem I can see 
> with running from the DB only is removing expired records. 
> any suggestions on the best way to remove expired records 
> from the DB without having the in memory list available would 
> be appreciated.

You would have to transverse the database at some interval and examine each
record in turn. Best to do only at module load since it would be expensive
if there is a potentially unlimited database size. At run time you could
expire old entries as they are reqested so that the database has some level
of run time update available. That should be sufficient for a first pass at
implementing such a system.

A couple other things I have noticed.

1) You are currently watching EVENT_NICKIP but treat it as a signon event.
This event is only issued on IRCds where NICKIP is not available when the
DNS lookup complete and you will be suggesting a SIGNON event happened at
some point after the user signed on and may have performed other more valid
actions such as channel join. Since seenserv does not use IP addresses I
suggest this event is removed.

2) You watch EVENT_KILL, EVENT_LOCALKILL,	EVENT_GLOBALKILL and
EVENT_SERVERKILL for kill processing. This means you will get double events
under certain conditions since both global and server kills will generate
KILL as well. If you want to merely record kill, I suggest using EVENT_KILL
and EVENT_LOCALKILL only since local kills will not trigger a KILL event. If
you want to differentiate between server and oper kills, use
EVENT_LOCALKILL, EVENT_GLOBALKILL and EVENT_SERVERKILL but do not use
EVENT_KILL to avoid the double event.

Mark.