-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log module #8847
Log module #8847
Conversation
3065208
to
88da2da
Compare
My main concern is the YAML file, for which I have some questions:
|
Re 1. the idea was that Re 2. I guess you could do I agree with 3. There should be an usable Crystal API for configuring |
Where is I see it can be set, read, etc., but I don't see it used. Also, when I do Is it there so that formatters can optionally use it? If so, maybe provide an example of such formatter? |
@ysbaddaden The @asterite I will add the output of the context to the stdio backend. Yes, there will be a |
@straight-shoota / @ysbaddaden . regarding 2. There are some method that are defined in In particular overriding |
I don't think customizing the setup process should require reopening stdlib classes to redefine methods. require "log"
Log.setup do |builder|
builder.bind "*", :info, SomeBackend.new
end |
The challenge is how the logs initialized in shards or std-lib will grab that configuration. If it is a method with proc the evaluation order is not error prone. We could have some macro to hide the redefinition of the method. But I don't think a plain proc will be enough. require "myshard"
# shard
module MyShard
Log = ::Log.for("myshard") # use the current configuration
end
# my app
Log.setup do |builder|
# ... but the MyShard::Log could be already initialized at this point
end |
I see. Making |
@straight-shoota the last commit changes
|
As I understand it, right now the setup needs to happen before you call Why is that so? Can't |
How you ensure the logs are configured properly for the very first entry? If we support reconfiguration of existing loggers we can mitigate a bit the issue, but still some logs might be emitted to invalid sources. So I think we need to setup loggers before any |
If we allow changing the log configuration at runtime, then everything is properly configured for the very first entry: with default values (I guess stdout). I don't think it matters much if a few log lines are written to an incorrect place. Usually one would configure logging before the main code. That's why I also think the Log configuration should be explicit in code, with something like: Log.configure do |config|
# ...
end If it's hidden behind a (the above snippet can also be |
In general, explicit > implicit. |
I agree the explicit would be clearer and more customizable. For example you could remove the need to set an ENV var or have a particular yaml file. You could do |
The datum macro seems very premature. Could it be introduced as a seperate PR, after this one and see what code it cleans up? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far, this is just a preliminary lookover.
f0bd31e
to
f406530
Compare
With the last push I removed Now a The Logs are tracked via weak refs in case to avoid leaking them. The builder is thread-safe. The top level docs are updated
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, I love it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍰
Thanks everybody for the feedback and the discussion on this topic. I'm happy with the outcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great PR, it was a bliss to read 👌
Thanks a lot for this!!
Here are a few comments for a later cleanup pr 🙃
Since the final state of the PR changed from the initial proposal, I wanted to submit an updated version of it. Although the docs are available as well. Generating logsThe are a couple of severity levels it can be used to emit logs: Usually you will type Log.info { "Hi there!" } Note: Using the result of the block for the message will allow the runtime to ignore possible conversions, string interpolations to build the message if it is not going be emitted. The When generating logs you will use a When defining a new shard or module you will define a module DB
Log = ::Log.for("db")
end Thanks to the relative resolution of constants, using If you move code around during development, you will not need to deal with If you need nested sources, you can use the parent namespace class DB::Pool
Log = DB::Log.get("pool")
end Or, if you want to hard-code it. class DB::Pool
Log = ::Log.get("db.pool")
end Or, you can pass directly a type and the source will be inferred from it. class DB::Pool
Log = ::Log.get(self)
end The result of Log.get("db").level = :verbose # or Log::Severity::Verbose
do_something_with_db
Log.get("db").level = :none # Log::Severity::None disables all levels
# DB::Log can be used instead of Log.get("db") Note: Setting the level is specific to a source and not it's nested ones. The previous example doest not affect the Note: Note that setting the level of a When building a program it will be possible to ignore completely as part of the compilation the You can also change programatically the logger level upon creation if you are hacking around. class DB::Pool
Log = ::Log.get(self, :debug)
end Writing backendsThe std-lib will come with a basic backends:
But you can define your own backend by inheriting
class MyCustomBackend < Log::Backend
def initialize
end
def write(e : Log::Entry)
end
def close
end
end Whether the write operation in sync or async it will be up to the implementation. There might be some base modules to implement easily an async backends (similar to ConfigurationConfigure logging explicitly in the codeUse You can indicate actual sources or patterns.
The following configuration will setup for all sources to emit
Configure logging from environment variablesInclude the following line to allow configuration from environment variables.
The environment variables The valid values for
The logs are emitted to If
Implicit ContextThe context information will live in the fiber. A new fiber will start with an empty context. Context are immutable but can be extended with convenient methods. The context are immutable so the backend can keep a reference to the context that was used when generating the The current context can be read from You can change the current context with Log.context.clear # => clears current context
Log.context.set request_id: 34, user_id: "[email protected]" # => extends the current context When spawning c = Log.context # current context
spawn do
Log.context = c
# ...
end
Log.context["request_id"] # => 23
Log.with_context do
Log.context.set request_id: 57689
Log.context["request_id"] # => 57689
end # previous context will be restored at this point
Log.context["request_id"] # => 23 Other typesDisclaimer: this is more the structure description rather than the actual definition. alias ContextValue = Bool | Int32 | Int64 | Float32 | Float64 | String | Time | Hash(String, ContextValue)
alias Context = Hash(String, ContextValue)
record Entry, source : String, severity : Severity, message : String, timestamp : Time, context : Context, exception : Exception?
enum Severity = Debug ; Verbose ; Info ; Warning ; Error ; Fatal ; None |
* Deprecate logger module * Replace logger for log in crystal playground
Implements Log module as described in
#5874 (comment)#8847 (comment) . With most of the documentation needed to use it.Replaces the logger usage in the compiler (the playground used it)
Mark logger module as deprecated.
I added an internal macro method to expand to types similar to JSON::Any and YAML::Any since I needed two of them.
Fixes #5874
Supersedes #6901