Hi, I guess it's my turn to give two and a half cents of worthless opinions: First, I think having a general logging facility is a good idea but I've got a few technical and a more philosophical issues. Technical things first:
- In my opinion, just renaming the current ParserLog class to OpmLog (or
Opm::Log) is a not so smart idea, because this class' API is very much
tailored to the parser module (what is the filename and the line number
in a general purpose context? the source file location of the call? not
really.)
- Dynamic log levels can be done, but I think if they only add a tiny bit of
API-level inconvenience compared to static log levels, I think they are not
worth it.
- Why does the ParserLog class need to be renamed at all? I'd rather see it to
be a frontend for the Opm::Log class which itself can be a singleton that acts
as a plumbing layer between frontends and backends. (the compiler guys call
this a "middleend" but that's about the most horrible name I've ever stumbled
upon in CS ;))
Now the non-technical concern:
- wouldn't it be better to wait until the Big Module Hierarchy Refactoring
(TM) is done so that the parser module does not need to host such general-
purpose infrastructure code? I mean, the dependencies between the modules are
already confusing enough as they are...
The rest of my comments are inline:
On Monday 15 December 2014 10:12:21 Joakim Hove wrote:
> > 1. The logging facility should be implemented as a singleton, i.e.
> > it should not be necessary to pass in a reference to a log class to all
> > methods/functions which might want to add a log message.
> I think this is a good idea, and in line with common usage of the singleton
> pattern. However, we should make a few things clear up front:
>
> 1a. This is not a universal logging facility in that we want to cover every
> possible use in any possible code, but tailored to what we need in OPM.
>
>
>
> Certainly - that was also part of the reason the name OpmLog gave slightly
> positive connotations.
I agree with Joakim that calling it OpmLog does not impose the impression that
it is a system-wide logging facility like journald or rsyslog...
> 1b. The logging facility should not be used for all communication with the
> user, only warnings/errors etc. I think that regular output should not be a
> part of this, even though that prompts more questions,
>
>
>
> Why not - that is my ambition? At least in a production setting the
> simulator will be run in queue system and all output must/will be
> redirected to a file. By having multiple backends we can send messages many
> places, including stdout with a common use pattern in the client code. My
> (maybe unrealistic ??) hope is that the logging system is so good that Joe
> Developer will rather sprinkle the code with a couple of
> "Log::addMessage()" instead "std::cout" while debugging/developing.
I think you both have different points: As far as I understand Arne does not
want
it to be used for interactive user communication or a non-interactive which the
user *must* read (i.e., fatal errors)...
> > 3. The log levels, i.e. Warning, Error and so on should be dynamic;
> > and each backend should have it's own loglevel.
> I do not quite understand what you mean by these being dynamic, and I do not
> think different backends should define their own. These levels are to be
> used in client code (simulators) where I can imagine such a thing as
> "Newton failure, report this at Warning level" is something the client
> would like to do. If the way to specify a Warning changes with the backend
> then there is no polymorphism here, so I fail to see how that could be
> done.
>
>
>
> What I mean; (which might still be a bad idea ?) is :
>
>
>
> 1. The global log handler manages a set of log levels (i.e. Warning,
> Error, Message, ....) dynamically. The default will of course be built in,
> but it should be possible from clientcode to say something like:
>
> size_t message_id = Log::addMessageType();
>
> ...
>
> ...
>
> Log::addMessage(message_id , "This message is tagged with the new message_id
> tag");
Static message priorities make things like
Opm::Warning << "foo!" ;
easy. I cannot really see how this can be done using dynamic ones...
> 2. When a backend is instantiated it is instantiated with a mask of
> the message types it accepts, i.e.
>
>
>
> ErrorBackend backend( Log.ERROR );
>
> MessageBackend backend( Log.ERROR || Log.MESSAGE || message_id ); // Will
> include the new message type
That should probably better be the job of "filters" which could be simple
classes
which exhibit an method
bool acceptMessage(int priority, LogFrontend *frontend, const std::string &msg);
combining arbitrary filters with arbitrary backends is quite simple in this
scheme...
> Each backend will make it's own decision whether to include
> the message.
that would make the backends quite convoluted as some backends may not only
chose messages based on their priority, but also based on the frontend which
generated it. This means that any backend needs to be potentially aware of all
frontends..
> Point 2, i.e. that each backend does It's own filtering decision I am quite
> certain is a good idea, point 1 is maybe overengineering?
I agree with both of the above statements, but I'd like to separate the
filtering
mechanism from the delivery schemes (i.e., the backends).
cheers
Andreas
--
the unix philosophy: do 90% of one thing, and barely do it adequately
-- Adam Jackson
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Opm mailing list Opm@opm-project.org http://www.opm-project.org/mailman/listinfo/opm
