Replace Log with stdlib logger #133

rkallos · 2020-06-07T11:49:58Z

Closes #127

rkallos · 2020-06-07T11:57:24Z

Upon further reading, should I change the loggers to all print to a single fd to also fix #121?

SeanTAllen · 2020-06-07T11:57:55Z

corral/util/log.pony

-     .> append(level.string())
-     .> append(": ")
-     .> append(msg)
+primitive LevelLogFormatter


Where is this used? I'm wondering how valuable it is at the point.

SeanTAllen · 2020-06-07T11:58:09Z

corral/util/log.pony

-
-interface val LogFormatter
-  fun apply(level: LogLevel, msg: String, loc: SourceLoc): String
+use "logger"

 primitive CodeLogFormatter is LogFormatter


Where is this used? I'm wondering how valuable it is at the point.

SeanTAllen · 2020-06-07T11:59:15Z

corral/util/log.pony


 primitive CodeLogFormatter is LogFormatter
-  fun apply(level: LogLevel, msg: String, loc: SourceLoc): String =>


I assume the log level where was so it would be polymorphic. Did you see that there's no need for being able to switch between these formatters?

Log level was there because log.pony had its own implementation of LogFormatter which accepted a log level argument. The log level argument was used in LevelLogFormatter to prepend the log message type to the message being logged.

Yup that makes sense now. I like how you moved the log level to being purely at the top level.

Some package level comments for the logging subsystem for corral that details how the different parts work together should be added the PR so that folks in the future won't be asking the same questions.

SeanTAllen · 2020-06-07T11:59:35Z

corral/util/log.pony


 primitive SimpleLogFormatter is LogFormatter
-  fun apply(level: LogLevel, msg: String, loc: SourceLoc): String =>


I assume the log level where was so it would be polymorphic. Did you see that there's no need for being able to switch between these formatters?

SeanTAllen · 2020-06-07T12:00:37Z

corral/util/log.pony

-     .> append(": ")
-     .> append(msg)
+primitive LevelLogFormatter
+  fun apply(level: LogLevel): LogFormatter =>


this seems odd. it only takes a log level?

How is this being used? The lambda concerns me. I'm not sure why that is needed.

If I understand the original code properly, the previous LevelLogFormatter prepended the log type (fine, info, warn, error) to the message being logged. It is only used in main.pony, either as a one-off before exiting or after parsing the command line arguments.

If you like, I could get rid of it by changing the call sites to add the log message type.

Ah so the existing pony standard library logger doesn't have the log level. That makes sense. So, this wraps every log call later and appends the level. That makes sense.

Can you add some doc comments to both the apply method and the primitive to make clear what it is doing?

SeanTAllen · 2020-06-07T12:00:53Z

corral/util/error.pony

@@ -1,4 +0,0 @@
-class val Error


was this not used?

I removed it because it clashed with logger.Error, and it only ever held strings to be passed to a logger.

Makes sense.

SeanTAllen · 2020-06-07T12:04:47Z

Upon further reading, should I change the loggers to all print to a single fd to also fix #121?

No. That's more of a "let's look into the interleaving and make sure its not a bug".

SeanTAllen · 2020-06-07T12:22:41Z

corral/bundle/bundle.pony

    dir = dir'
    log = log'
    info = InfoData(JsonObject)
-    log.info("Created bundle in " + dir.path)
+    log(Info) and log.log("Created bundle in " + dir.path)


here's a question, should there be a "Log" class for corral that keeps the previous form of different methods and it contains the Logger actor and the check to see whether to send a message? It's certainly cleaner and hides to log(Info).

By hiding that, we could have single comments per method on the Logger class that explains why that is done to short circuit sending a message to the actor.

It might make the code base more approachable for folks who don't really know Pony.

Thoughts?

I'm not sure how that would come together with the other changes. Perhaps it might be problematic. I'm not sure.

I looked into that approach, but when I read the docs for the logger package and noticed the use of boolean short-circuiting to avoid the work of building strings, I didn't see a clear way to preserve the encapsulation provided by Log and to get the near zero-cost logging offered by the logger API. That's why I chose to replace all the call sites with the idiomatic use of logger.

Makes sense.

It's amusing that you are reminding me of this given that I wrote those docs for the logger package after writing the logger package.

😆

SeanTAllen · 2020-06-07T12:32:53Z

This looks good aside from needing some documentation for the corral logging package and a couple of the "trickier" formatter usages.

SeanTAllen · 2020-06-07T12:37:05Z

@rkallos this will need to be rebased as it now has conflicts with changes in cmd_update.pony

rkallos · 2020-06-07T12:45:17Z

Rebased 😃

SeanTAllen · 2020-06-07T12:46:51Z

Looks like you have compilation errors after the rebase.

rkallos · 2020-06-07T12:47:48Z

Note to self: Run tests locally after rebasing.

SeanTAllen · 2020-06-07T12:50:41Z

@rkallos ping me after you've had time to add the documentation/comments and I'll have another look. I might be delayed as I'm moving next weekend and most of my time is going towards work and the move at the moment.

SeanTAllen · 2020-06-07T13:32:12Z

corral/util/log.pony

-     .> append(msg)
+primitive LevelLogFormatter
+  """
+  LevelLogFormatter produces LogFormatters that prepend a log level to entries.


This doesn't really feel like explains the purpose.

We want to have the log level in all the output for filtering, searching etc that folks might want to do with an unstructured log.

Rather than adding the format to each string at all call locations, we create one of these in main that in turn wraps the other formatters appending the log-level"

That's more than the comment for this primitive, but part of that should be included in this primitive.

I'd like to see log.pony moved from corral/util to corral/log or corral/logging and have a package level doc as well that explains the relationship between this logger and the others and how logging is in general used in corral (as detailed above).

Including the discussion about the short circuit that your commit switched to.

Basically cover everything that I felt the need to ask you about as part oft this PR.

Thank you for the clarification. I've added some more clarification. Let me know what you think.

SeanTAllen · 2020-06-07T14:10:20Z

corral/log/log.pony

+work of building strings that won't get logged. This can make log statements
+somewhat verbose:
+
+```


can you pony codefence type?

Done. It's pony, not ponylang, right?

SeanTAllen · 2020-06-07T14:13:33Z

corral/log/log.pony

+  LevelLogFormatter is used by the logger(s) created in Main.
+  """
+  fun apply(level: LogLevel): LogFormatter =>
+    """


Lines 33 to 35 should be most of the apply method comment.

Creates lambda to... blah blah blah.

SeanTAllen · 2020-06-07T14:13:52Z

corral/log/log.pony

+  sites by creating a special-purpose LogFormatter whose sole purpose is to
+  prepend the log level.
+
+  LevelLogFormatter is used by the logger(s) created in Main.


information from 33 to 37 should be in the package level comments.

SeanTAllen · 2020-06-07T14:15:44Z

corral/log/log.pony

@@ -0,0 +1,64 @@
+"""
+# Logging in corral
+


I think the most important thing for the package docs is to detail the various components and how they interact so...

uses Pony standard logging.

There's a log level formatter that acts as a wrapper around others so that we get INFO etc output without having to spread throughout the codebase.

And here is how it is used with other formatters.

SeanTAllen · 2020-06-07T14:17:53Z

I left some more docs comments. I think what is really missing is, particularly at the package level is...

Written for someone who is coming in to modify logging, add new functionality.

Description of how logging is done and works.

How formatters interact with one another.

What each formatter does.

Basically what you need to be aware of to make changes.

I hope that makes sense. Not sure if it does.

SeanTAllen · 2020-06-07T14:36:27Z

I had to run an errand... that lead to a thought.

For the documentation, there are two audiences.

People who want to enhance/change the logging that exists.
People who are going to add logging to new parts of corral.

The basic informational needs of both groups should be met.

rkallos · 2020-06-07T14:44:12Z

Upon further reflection, I think my approach with LevelLogFormatter is incorrect. It prepends the same log level to all log messages, regardless of user intent:

let logger = StringLogger(Info, env.err, LevelLogFormatter(Info))
logger(Error) and logger.log("some error message")
// logs "INFO: some error message"

To fix this, I think I will introduce a new primitive that accepts a log level and a string and does the prepending there. The resulting code should look like this:

let logger = StringLogger(Info, env.err)
logger(Error) and logger.log(LogMsg(Error, "some error message"))
// logs "ERRR: some error message"

This avoids the downside of having Log be a sort of wrapper class around Logger (not being able to save on work via short-circuiting) while also removing the need for LevelLogFormatter.

Alternately, this might make a compelling argument for adding a log_level argument to the LogFormatter interface in the standard library, though that would require a great deal of care; it seems like it would be a breaking change.

SeanTAllen · 2020-06-07T14:48:07Z

@rkallos This is a good real world use case for the standard library logger. I think it might have highlighted a need for the inclusion of Log Level in the LogFormatter stdlib interface.

Would you be up to opening an RFC to make that change? It's small but we need to run it by people. I'm fully in support of it.

This PR could then be updated so that it would be a small change to finish off and merge once the standard library has been updated.

rkallos · 2020-06-07T15:29:13Z

New RFC

rkallos · 2020-07-31T22:29:26Z

Now that 0.36.0 is out, work on this can continue. It'd probably be best if I started over, as a lot of the details are now lost to me.

SeanTAllen reviewed Jun 7, 2020

View reviewed changes

rkallos force-pushed the use-stdlib-logger branch from 8393e2a to 23800fd Compare June 7, 2020 12:44

rkallos force-pushed the use-stdlib-logger branch from 23800fd to c99ef75 Compare June 7, 2020 12:48

SeanTAllen reviewed Jun 7, 2020

View reviewed changes

rkallos force-pushed the use-stdlib-logger branch from 627590c to c5545fd Compare June 7, 2020 14:10

SeanTAllen reviewed Jun 7, 2020

View reviewed changes

hasheddan mentioned this pull request Jun 14, 2020

Make corral less verbose #137

Merged

rkallos force-pushed the use-stdlib-logger branch from c5545fd to a77f4d9 Compare July 31, 2020 22:24

rkallos closed this Jul 31, 2020

rkallos force-pushed the use-stdlib-logger branch from a77f4d9 to fb97117 Compare July 31, 2020 22:28

rkallos deleted the use-stdlib-logger branch September 23, 2020 12:20

ergl mentioned this pull request Jun 30, 2021

Migrate to stdlib logger #185

Merged


		primitive CodeLogFormatter is LogFormatter
		fun apply(level: LogLevel, msg: String, loc: SourceLoc): String =>


		primitive SimpleLogFormatter is LogFormatter
		fun apply(level: LogLevel, msg: String, loc: SourceLoc): String =>

Replace Log with stdlib logger #133

Replace Log with stdlib logger #133

Conversation

rkallos commented Jun 7, 2020

rkallos commented Jun 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Jun 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Jun 7, 2020

SeanTAllen commented Jun 7, 2020

rkallos commented Jun 7, 2020

SeanTAllen commented Jun 7, 2020

rkallos commented Jun 7, 2020

SeanTAllen commented Jun 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Jun 7, 2020

SeanTAllen commented Jun 7, 2020

rkallos commented Jun 7, 2020

SeanTAllen commented Jun 7, 2020

rkallos commented Jun 7, 2020

rkallos commented Jul 31, 2020

SeanTAllen commented Jun 7, 2020 •

edited

Loading