Re: Custom log file

[Jan-Henrik Haukeland]

Here is a more detailed description of my objections to you patch Marco,

1. The custom log function does not integrate with the monit code, it  
intercept monit's event handling by inserting a call out to its own  
logger module. This is not integration, this is a control flow hack  
to inject your own code. This is what you do when you have 5 days to  
hack monit to provide a certain functionality you need.

2. The customlog.c module consists mainly of cut&past code from log.c  
and event.c and is a blatant duplication of code. When you duplicate  
code like this, red lights should go off and warn you that you have a  
design problem. The main functionality lays in the main while loop  
that parse the log format. This code may be useful, but not in this  
design. A minor detail is also that the code is peppered with  
insecure buffer handling functions, boundary checks *must* always be  
used, i.e. use strncpy instead of strcpy and so on.

3. Finally I also have a major problem with how the custom log stuff  
manifest itself at the control file level. You may recall that I had  
my objections when this was first proposed by you. Here is one example,

    if failed port 80 protocol http then alert log "message" alarmid 33
        severity warning

What's going on here is that an event with a certain alarmid and  
severity are hardcoded to be logged. What if I want this kind of  
events to be sent in an email instead? or as a snmp message? There  
are no flexibility for choosing how you want to handle an event with  
severity warning or with alarmid 33. This is bad design, because  
implementation details shows up at the configuration level. You don't  
want that, to do this proper, you may tag a certain event with e.g. a  
severity and alarmid and allow the user to configure where he want  
those alerts to be sent. For instance,

    if failed port 80 protocol http then alert with alarmid 33 and  
severity warning

The user could then configure if events with severity "warning"  
should be logged, sent as a snmp message or posted to m/monit not  
just logged. Well, monit does not support this flexibility yet, but I  
don't particularly want to maintain code that is orthogonal to such a  
design.

As for the implementation details with regards to logging and using  
several log files. You do not duplicate code, but write one and only  
one general Logger "class" which can be used in different contexts.  
This was what I had hoped for with regards to the patch. Pseudo code  
below shows what I had in mind.

/* Initialization, setting up log delegators */
List_T loggers= List_new();
Logger_T errorLogger= Logger_new("/var/log/monit/error.log");
Logger_T customLogger= Logger_new("/var/log/blabla.log");
Logger_setLogFormat(customLogger, "%D, %T, %A, %S, %M, %T");

loggers_add(errorLogger);
loggers_add(customLogger);

....

/* Perform logging upon event handling */
for(int i= 0; i < loggers_length(loggers); i++)
    Logger_log(loggers_get(i), Event_T e);



Finally, I want to stress that your contribution is highly  
appreciated and I really hope that the above is not taken badly by  
you. We *do* value any input especially those backed up by code.  
Likewise I hope you can appreciate that we as non-paid maintainers of  
the monit code may have a different agenda than you on some issues.  
You obviously had a scratch




On 2. jun. 2005, at 17.13, Marco Ermini wrote:

> On 6/2/05, Jan-Henrik Haukeland <address@hidden> wrote:
>
> > Thanks, for the patch[1] I think we can and will use code from this,
> > but not accept the patch as is[2]. The code looks good but IHMO the
> > main problem with the patch is that it is designed as an add-on and
> > does not integrate well with either logging and the event handling
> > mechanism already in monit.
>
> I don't catch it. It is written just for Monit and modeled for it. It
> has no sense out of Monit. It integrates perfectly with the event
> handling mechanism (it is even called inside the same spinlock; I did
> not modify the Event_post() function just to not impact too much on
> the code). As I already explained SEVERAL times, it has NOTHING to do
> with the Monit log mechanism - it cannot integrate with a totally
> different topic :-)
>
> I really don't understand this point...
>
>
>
> > Instead it implements it's own module
> > where logging and "events" are tightly coupled into one separate
> > unit. In fact the code is so standalone that you could probably take
> > it and add it to any program without to much trouble :)
>
> I understand this, but I don't agree. Maybe the customlog.c file could
> be "recycled", but it has no real sense out of Monit. The "events" are
> just the Monit "events".
>
>
>
> > This is of
> > course not necessarily a bad thing by all means. However, I feel that
> > if we should incorporate the patch directly we will bind us up into a
> > design that is not as flexible as we would want to have.
>
> Sorry, maybe it's my limit, but I don't understand this "design"
> thing. It does not introduce any design changes in the way Monit
> works. It just executes another function whenever after (or just
> before, I can't remember?) an Event_post() is executed. The majority
> of the time I spent is to find the correct way to integrate it as
> smoothly as I could in Monit - the functionality in itself is quite
> simple (not very simple, but it tooks a maximum of 1/4 of the time
> itself, 3/4 of the time was studying smooth Monit integration).
>
> I would suggest at the people who are expressing their opinion, to
> _really_ look at the straight source code, without take other people's
> opinion as granted...
>
> If you may arise a critic to this patch, it is that is not generic
> enought. You may extend it by allowing several logs to be created, and
> allowing the user to customize every log. But you certainly need
> *different* log files for different purposes. And you *don't* want to
> mess the actual Monit log (at least not with all the functionality I
> provided).
>
> I could assure that I accurately listened and evaluated your opinion
> about it before starting this patch... of 6 days of development, the
> first 2 was about studying Monit source code. I think that you can't
> simply apply a customization to the "actual" Monit log: when Monit
> outputs its own log, in many cases it is simply happening nothing
> "interesting" (interesting as to be customized). When it can't
> daemonize because the tcp port is already occupied, or can't start
> because the monitrc is missing, or can't write a log because the disk
> is full... you are not running an Event_post(), so the vary majority
> of the customization parameters has no sense. These are all cases in
> which automation didn't apply, the user have to manually check what is
> happening and manually fix the problem. In my opinion you want to
> customize the log only on Event_post() cases; customize the Monit's
> own log for _every_ case, it would have certainly required much more
> effort than my patch had (and this was a great motivation...
> sorry!!!), you had to introduce many checks which will waste CPU time,
> and it definitively will mess up more things than my own patch :-)
>
> Anyway, if you find that "generalizing" my patch could help it to be
> introduced in Monit mainstream source code, I could certainly consider
> it. Just let me know the details in which you want me to do it, and I
> will follow them strictly :-)
>
>
>
> > What we need is a clean separation between the event handling
> > mechanism and logging. The way I see it, event handling should be
> > clearly decoupled from any output channel, such as logging. That way,
> > it is much easier to provide an "event manager" that can delegate
> > (see the delegation design-pattern) output to different output
> > handlers. Such as sending an email, send a snmp trap, or log a
> > message in a log file.
>
> This is a totally different effort and it has nothing to do with "my"
> patch. It sounds to me as an excuse :-)
>
> If you rejected the patch because it collides with a "supposed" design
> which is not actually in place... I have no magic crystal ball to look
> into :-)
>
>
>
> > As I said, your code is fine and if you allow us to use parts of the
> > code especially for the log format it would be appreciated. I would
> > also certainly like to thank you for providing this patch and for
> > using time and energy on the work. If the patch works for you and
> > your company it's great, that's what open source is. You take the
> > code and tweak it to solve a particular problem and if others can use
> > it is a nice side-effect.
>
> As I said, it's not a "personal" effort but, for sure, something
> everyone running a fairly decent hosting center will need. It's
> something Apache has by _years_, and not by just a case.
>
> In any case, my company is Vodafone, and we are talking about
> 20,000,000 users, several hosting centers, several applications
> monitored ;-)
>
>
>
> > [1] It's not a patch but a copy of the CVS, the best thing is to
> > provide changes as a real patch. For instance by using, [cvs diff -u]
>
> It is something you could deal with - or not? ;-) - but that I could
> not deal with.
>
> Sorry, but CVS sucks great... I would use it to extract source code
> periodically, or in conjunction with cvsup, but no more than this... I
> could program everything you need in C - especially Monit was quite
> enjoyable, because it is very well written, it is object orientation
> done in pure C: it is something I especially like and approve ;-) -
> but I will send it to you as is :-P - then you could do whatever you
> want with it, I am not a touchy person :-)
>
>
>
> > [2] I speak for myself and other commiters in the project may very
> > well have a different opinion. In that case we will vote.
> [...]
>
> > --
> > Jan-Henrik Haukeland
> > Mobil +47 97141255
>
> Bah. I don't like voting. I like rational discussions. I like to
> listen at YOUR opinion, which is valuable for me :-)
>
> Do you know why the Biblic table has just these ten, very clear and
> precise rules? because they are not deliberated by a forum :-)
>
> The patch is totally your, you can do it whatever you want, really -
> as I said, I am not touchy :-)
>
> I reached my target and I think that it could be useful to many
> others. If I can put some more effort to realize what you like, and
> together we realize something which is generic enought to reach both
> (me and your... others?) preferences, I will do it :-)
>
>
> Bye!
> --
> Marco Ermini
> http://www.markoer.org
> Dubium sapientiae initium. (Descartes)
> address@hidden # mount -t life -o ro /dev/dna /genetic/research
> << This message is for the designated recipient only and may contain
> privileged or confidential information. If you have received it in
> error, please notify the sender immediately and delete the original.
> Any other use of the email by you is prohibited. >>

--
Jan-Henrik Haukeland
Mobil +47 97141255