Revert "Implement RFC0067"

[rkallos]

This reverts commit 4d4cb92.

The original intent behind RFC0067 was to make it easy to add a prefix with the
log level for any given log message. However, there is a logic bug in the
implementation; the log level prefix added to messages in LogFormatter would
always be the log level specified in Logger.create(), not the log level desired
at specific call sites. This would lead to misleading log messages where the log
level prefix doesn't match the severity of the log message, such as "FINE:
Critical error. Shutting down".

It would be better if these changes were reverted so a more thought-out solution
could be implemented in the future.

[jasoncarr0]

I'm uncertain that this is the issue with your changes. That is to say, the issue is rather that the Logger class does not seem to take a level argument on application, and I believe that is the ultimate fix (even before reverting this, although I could see the argument that it is misleading).

[SeanTAllen]

I'm in favor of this change. The logger that I designed that is part of the standard library doesn't make sense with this change. There are larger changes that could possibly be done to it (although I don't think they should be done). I think rather that we accept it as a simple logger that is in the standard library for now and others can write different log packages if they want.

[SeanTAllen]

@jasoncarr0 to do as you suggest would mean a few possible things:

sending all possible log messages to logger actor (lots of overhead) and then throwing a bunch away.
the logger was designed to explicitly avoid this.

keep it as is but also provide a log level that is part of the message. however, i could then have a mismatch in log level between the one that is used as a short circuit to not send messages and the one that is used for display.

given the design goals of the logger as I built it, the RFC should be reverted. It doesn't make sense within the design of the logger as it stands and it's goals.

[jasoncarr0]

@jasoncarr0 to do as you suggest would mean a few possible things:

sending all possible log messages to logger actor (lots of overhead) and then throwing a bunch away.
the logger was designed to explicitly avoid this.

keep it as is but also provide a log level that is part of the message. however, i could then have a mismatch in log level between the one that is used as a short circuit to not send messages and the one that is used for display.

given the design goals of the logger as I built it, the RFC should be reverted. It doesn't make sense within the design of the logger as it stands and it's goals.

I don't think that the goals of the logger are mis-aligned, but that the addition of a log level to a message is an important piece of info with logging in general and is used in practice (hence why this original RFC came about). We can definitely chat about that elsewhere and if we should make other changes to the logging interface.

[SeanTAllen]

@jasoncarr0 In order to do that you need to have:

logger(Warn) and logger.log(Warn, name + ": " + reason)

note the required duplication of Warn. That is an awful API because now it could be:

logger(Warn) and logger.log(Info, name + ": " + reason)

and the level-logged as no connection to the level actually used.

The logger as I designed it has no ability for log to know the level that was used to decide if logging should be done. I designed in order to maximize performance by

• Minimizing messages sent to the logger
• Not doing string concat etc if the message won't be logged

It is my opinion that the RFC should be reverted and other log packages could be developed and it could reasonably be argued that the current one should be moved out of the standard library to allow others to flourish.

I'm well versed in this. I wrote the Logger. I asked Richard to write the RFC in question which was crafted to my specification. The problem was that the RFC add anything to this logger implementation. It was a mistake.

Whether or not the log level should be in the message is an entirely different question. I agree that some folks would want it. Most folks probably. The tradeoff that was made in the Logger that I wrote was "performance over all else". For folks for whom that isn't a good trade-off, they want a different logger with different trade-offs. The one in the standard library is designed completely for performance with zero affordances for much else.

I stand by my statements that the goals and API of the existing logger are mis-aligned with the RFC.

[SeanTAllen]

There's an important lesson from RFC 67.

Neither Ricard nor I actually made the change prior to writing the RFC and trying it out. If we had, we would have seen that the RFC was implementing something that didn't accomplish it's goals. The moment that Richard starting to trying to make the change in Corral that led to the RFC, it was immediately obvious it didn't accomplish the goal.

[jasoncarr0]

@SeanTAllen to pull this discussion out, I've made a small issue on the rfcs repo at ponylang/rfcs#181

[SeanTAllen]

Thanks @rkallos.