-
Notifications
You must be signed in to change notification settings - Fork 69
Context api #132
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
base: master
Are you sure you want to change the base?
Context api #132
Conversation
jeremyevans
left a comment
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.
I think for most of the cases where you would want to add context like this, it may be better off logging in a more structured format (typically JSON), so you can provide the context as an entry in the JSON object. We could consider a Logger::JsonFormatter that outputs log messages in JSON. However, I'm not opposed to the general idea in this PR of supporting logging with context when doing plain text logging.
lib/logger/formatter.rb
Outdated
| context = if kwargs.empty? | ||
| "" | ||
| else | ||
| kwargs.filter_map { |k, v| "[#{k}=#{v}]" unless v.nil? }.join(" ") << " " | ||
| end |
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.
This should be moved to a format_context method, so it can be overridden in subclasses (similar to format_datetime).
Removing keys with nil values seems like a reasonable default, though having [] surround each pair seems odd. It also seems odd to add an extra space if the kwargs are empty after filtering as opposed to be empty before filtering. Not escaping values can result in potential log injection attacks if the value contains ][. Also true for keys, though you would hope something like this is not called with attacker-defined keys.
I think it would be better to use compact! instead of filter_map. I think it also may be good to use an append only approach:
| context = if kwargs.empty? | |
| "" | |
| else | |
| kwargs.filter_map { |k, v| "[#{k}=#{v}]" unless v.nil? }.join(" ") << " " | |
| end | |
| kwargs.compact! | |
| if kwargs.empty? | |
| context = "" | |
| else | |
| context = String.new << "[" | |
| kwargs.each do |k, v| | |
| context << " " unless context.bytesize == 1 | |
| context << k.to_s << "=" << v.inspect | |
| end | |
| context << "] " | |
| end |
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.
Moved to format_context. I haven't picked up on your suggestion yet, only because I'd like to first align on the format of context for this particular default formatter, before committing on a strategy.
Your suggestion would result, for the {a: "b", c: "d"} context, in the following: [a=b c=d]. I don't have a strong argument if others prefer that, but my approach was based on rails tagged logging, which is what I'd expect most to be familiar with, and for which I'd expect some level of tag-level search support in tools built around it.
LMK if this makes sense.
97c6eca to
377cfd9
Compare
|
@jeremyevans thx for the initial review 🙏
Indeed, in the vast majority of the cases (including the ones I linked to in the related issue), structured JSON logging is the main and most widely use case for extra context. But different logging formats is something that the
A JSON formatter could be interesting, but I'd prefer doing it as a separate PR. I think that there are already many "standards" for JSON structured logging (logstash being the one I'm most familiar with), and I'm not sure whether On another topic, I've added support for context both as array and hash. This is mostly because the former is used for rails tagged logging, and could perhaps be of some use there. Not sure if anyone from the rails core team could also have a look here to validate that, otherwise supporting both formats could be considered overkill. |
5cbe4d1 to
0af6de2
Compare
jeremyevans
left a comment
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.
This looks better. Obviously, this needs tests to be considered. I don't think the code submitted was ever tested, due to the obvious errors in it.
Note that Fiber.current is not available by default in Ruby 2.5. You need to require "fiber" to make it available.
Supporting only array and hash contexts seems odd to me. We should either support arbitrary contexts, or only hash contexts, I think. Support only hash contexts could be implemented by using keyword splat instead of positional argument.
I'm not sure it is worth supporting both block-based context and per-log-message context. It would be easier to support only block-based context, and only if the log formatter supports it. We could add this code:
class Logger
def with_context(context, &block)
formatter.with_context(context, &block)
end
endAnd move the with_context method to Logger::Formatter. That would result in no interface change between the logger and formatter. Custom loggers wouldn't support context, but would remain backwards compatible. The logger wouldn't need to check whether the formatter supported context.
e6032a8 to
5f0ff64
Compare
|
@jeremyevans again, thx for your thorough review 🙏
Indeed, this was submitted as a "proof of concept" of the proposal in the issue, not intended to be accurate. However,given
"fiber" is already being required in
It's definitely something I considered in an earlier comment of mine, but bear in mind that one of the use cases for this API would be to "import" some of the functionality of rails tagged logging into the stdlib fold, and that uses tags as an array of strings. Ideally, I'd like to have the opinion of someone from rails core to vouch for the usefulness of this idea though, otherwise I agree that narrowing the scope to only hashes would probably be reasonable.
I'd be more verbose IMO. Could make sense, but just for the same of discussion, if you look at # init
logger = Logger.new(STDOUT, level: 0)
# reset
logger.level = 1
# scope level
logger.with_level(2)So context would add a new idiom ( But this is also the reason why I don't think that context handling should be offloaded to the formatter. It makes sense from a backwards compatibility angle, but that type of state belongs IMO to the logger (formatter should be stateless, and could be shared across logger instances). |
2e1f0ce to
5912f09
Compare
|
@jeremyevans @nobu @sonots do you have any more feedback? What's missing in order to make this mergeable? |
|
In terms of moving forward, I'm not the logger maintainer, so I can only review and offer general advice. In regards to merging, you would need approval from @sonots. If after a reasonable amount of time (maybe 3 months) @sonots does not respond, you could add a Redmine issue and add it as a topic to be discussed during a developer meeting. My feelings on the implementation haven't changed much:
|
|
I'm not a maintainer either, but I do want logging context added to core. And I've been using a homegrown version of my own in production for over 12 years. I actually did mine almost entirely via the formatter (like @jeremyevans suggested) and that works okay... but looking at other structured logging libraries (in ruby and in other languages), I actually think it makes more sense to have the I don't have a strong opinion about My much bigger concern relates to @jeremyevans's other comments about supporting different context types (Hash vs Array vs ???). I would strongly recommend simplifying this PR to only support context as a Hash, and simplifying the formatter to drop rails style tags and only a support a basic structured logging-ish style. Rails style "tags" can easily be added to subclasses: just have a special case for Related to the above points, unlike this implementation, my private implementation "cheats" by declaring that the message parameter can be a hash everywhere, it merges |
nevans
left a comment
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.
I typed these review comments up a few weeks ago, but never sent them in. They assume we continue with the API change where context is sent in as a keyword argument. But, we could also reduce the backwards-incompatible API changes by allowing message parameters to combine context and message.
| context.dup | ||
| end | ||
| end | ||
| end |
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.
If we assume that context is always a hash, this can be simplified to something like:
| end | |
| # Override in subclass to allow non-Hash context types | |
| private def merge_context(prev_context, context) | |
| ctx_hash = Hash.try_convert(context) \ | |
| or raise TypeError, "can't convert %s into Hash" % [context] | |
| prev_context.nil? ? ctx_hash.dup : prev_context.merge(ctx_hash) | |
| end |
| # Default formatter for log messages. | ||
| class Formatter | ||
| Format = "%.1s, [%s #%d] %5s -- %s: %s\n" | ||
| Format = "%.1s, %s[%s #%d] %5s -- %s: %s\n" |
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.
IMO, we should combine message and context and leave the format string unchanged:
| Format = "%.1s, %s[%s #%d] %5s -- %s: %s\n" | |
| Format = "%.1s, [%s #%d] %5s -- %s: %s\n" |
Just for a quick visual scanning of logs: the more static fields don't vary much in length, so they should stay before context, which is going to vary in length much more.
Also, although non-local context is often relatively static and worth putting before the message, which context fields should be prioritized over the message depends on the application. IMO, the best generic solution is to print all the context after the message. For example, that's what the go stdlib structured logging package does: with the default formatter, slog.Info("hello", "count", 3) prints out 2022/11/08 15:28:26 INFO hello count=3.
Ideally, custom formatter subclasses should be able to trivially print context before, after, or around the message.
| def format_context(context) | ||
| context_str = case context | ||
| when Hash | ||
| context.filter_map { |k, v| "[#{k}=#{v}]" unless v.nil? }.join(" ") | ||
| when Array | ||
| context.filter_map{ |v| "[#{v}]" unless v.nil? }.join(" ") | ||
| else | ||
| context.to_s.dup | ||
| end | ||
|
|
||
| context_str << " " unless context_str.empty? | ||
|
|
||
| context_str | ||
| end |
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.
I agree with @jeremyevans that the default formatter should simplify by only allowing Hash context. And, as I wrote in my other comments, format_context shouldn't be concerned with trailing or preceding whitespace. Also, although I do like the style, the default formatter shouldn't surround context fields with brackets (IMO).
With those simplifications, I'd prefer to spend the default formatter's "complexity budget" elsewhere. 😉
- Extracting more more
format_{type}methods provides obvious extension points for common subclasses patterns. - simply joining
"#{k}=#{v}"is too naive, IMO. IMO, context values are much more likely than messages to include sensitive or externally sourced values (and thus be a source of security issues). So I think we need some form of built-in escaping, and an easy extension point to add filtering.
def format_message_with_context(msg, ctx)
join_fields msg2str(msg), format_context(ctx)
end
def join_fields(*fields)
fields.reject {|f| f.nil? || f.empty? }.join(" ")
end
def format_context(context)
return unless context
context.filter_map {|k, v| format_pair(k,v) }.join(" ")
end
def format_pair(k, v)
"#{k}=#{format_value(v)}" unless v.nil?
end
def format_value(value)
# "redundant" interpolation to avoid crash if to_s doesn't return a string
# https://github.com/ruby/spec/blob/3affe1e54fcd11918a242ad5d4a7ba895ee30c4c/language/string_spec.rb#L130-L141
value = "#{value}"
value = value.dump if value.match?(/[[:space:][:cntrl]]/)
value
endThis enables subclasses like the following, that's kind of like ActiveSupport::TaggedLogger.
class TaggedLogFormatter < Logger::Formatter
PRE_MSG_CTX = %i[user_id request_id controller action].freeze
# add brackets to each context pair, and treat key like a tag when value is true
def format_pair(k, v)
return format_tag(k) if v == true
(str = super) && "[#{str}]"
end
# print some context before the message and some after:
def format_message_context(msg, ctx)
return msg2str(msg) unless ctx
fields = Array(ctx[:tags]).map { format_tag(_1) }
fields << format_context(ctx.slice(*PRE_MSG_CTX))
fields << msg2str msg
fields << format_context ctx.except(:tags, *PRE_MSG_CTX)
join_fields(*fields)
end
def format_tag(tag) = "[#{format_value _1}]"
endOr, here's one that's kinda like logfmt for msg and context, but it uses the (more compact) default format for the other standard fields.
class NotQuiteLogFmtFormatter < Logger::Formatter
# Use ISO8601
def format_datetime(time) = time.utc.iso8601(6)
# Formats as msg="my message"
# or error="error messege [MyError]\n/path/to/file.rb:123 in 'foo'\netc..."
def msg2str(msg)
name = msg.is_a?(Exception) ? "error" : "msg"
format_pair(name, super(msg))
end
# flip the order: prints msg="my message" before context
def format_message_context(msg, ctx)
join_fields msg2str(msg), format_context(ctx)
end
# Standardize formats for Float, Time, #to_ary
def format_value(value)
case value
in String then super
in Float then format("%.3f", value)
in Time then format_datetime(time)
in ->{ _1.respond_to?(:to_ary) then format_array(value)
else super
end
end
def format_array(array)
format_value "[#{Array(array).map {|v| format_value(v) }.join(", ")}]"
end
end(I haven't tested these at all... only typed them here in the github comment box, so... they probably don't work as-is.)
lib/logger/formatter.rb
Outdated
| def call(severity, time, progname, msg) | ||
| sprintf(Format, severity, format_datetime(time), Process.pid, severity, progname, msg2str(msg)) | ||
| def call(severity, time, progname, msg, context: nil) | ||
| sprintf(Format, severity, format_context(context), format_datetime(time), Process.pid, severity, progname, msg2str(msg)) |
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.
Echoing my Format string comment here: by default the context should be adjacent to the message, after the always present (nearly) static width fields.
| sprintf(Format, severity, format_context(context), format_datetime(time), Process.pid, severity, progname, msg2str(msg)) | |
| sprintf(Format, severity, format_datetime(time), Process.pid, severity, progname, format_message_with_context(msg, context)) |
Ideally, custom formatter subclasses should be able to trivially print context before, after, or around the message. And, for that to work, format_context shouldn't be concerned with trailing or preceding whitespace. The caller should handle that.
While that merger could happen here, common subclass patterns can be simplified by extracting some more methods. I suggest pushing msg2str and format_context into a new method and letting that method merge context and message. I'll add a possible #format_message_with_context implementation to my comments on #format_context.
|
@HoneyryderChuck I fixed up my review comments and pushed a branch with them here: master...nevans:logger:context-api. I also fixed the tests for ruby 2.5 and 2.6, added some rdoc, made the other severity methods match |
|
Thank you both so far for all the feedback! I resumed the raised concerns into the following list:
I'll add some thoughts, with the intent to document pros and cons for others joining the conversation. 1If I understand the suggestion, it's about reducing the footprint and complexity of the change. It's a valid reason, and 2I agree that putting it in the formatter could make backwards compatibility simpler. Conceptually, formatters don't keep state around (the message, as well as other supported metadata, are passed as argument), and managing it is a bit beyond its scope IMO. For the sake of argument, looking at the gems mentioned above, 3I agree that focusing on one could make the change simpler. But the applied principle was the same as in 1, i.e. there's a known and ancient API (rails tagged logging) which supports arrays, and ideally simpler programs requiring similar tagged logging wouldn't need to jump through hoops and install 4I'm not sure how putting tags at the end, like go does, would be a sensible choice. For once, go's structured logging format isn't known to rubyists (who do not know go), so it'd be just another standard. And looking at the output, it isn't clear to me whether it was generated from 5I understand the appeal of the idea, but it wouldn't IMO compose well for other potential formatters, i.e. a json formatter would expect to dump the context "as is" into a function like Bear in mind that I'm not fundamentally against any of the suggestions (perhaps starting small could validate the rest), but I'd like to have more inputs before trimming the implementation. In order to spare you more back and forth, and because I suspect that getting feedback from @sonots will take more than 3 months, I'll go ahead and open an issue in the bug tracker and hopefully gather some more opinions around the loose ends. Compatibility with ruby 2.5 was restored. |
5912f09 to
3910bf9
Compare
Not really a must :). And when I say "like go's context", I'm speaking more by analogy, because there's a lot about it that annoys me. Go's But actually, there's an interesting distinction here for our purposes. And And that's a pattern I've used myself, a lot: different logger for different classes. Some of the logged fields come from the execution context, and other logged fields come from the logger itself. So I added another commit to another branch to implement that: master...nevans:logger:default-context 🙂 In that branch, you can assign a "default context" via |
|
@nevans default context is definitely a great idea, and something I considered implementing along the way. I'll make sure to cherry-pick your change. |
|
@HoneyryderChuck to (verbosely) address this point:
Firstly, you aren't expected to use the default handler in production. They include a pure logfmt handler and a JSON handler: slog.Info("hello", "count", 3)
// 2022/11/08 15:28:26 INFO hello count=3
logfmt := slog.New(slog.NewTextHandler(os.Stderr, nil))
logfmt.Info("hello", "count", 3)
// time=2022-11-08T15:28:26.000-05:00 level=INFO msg=hello count=3
json := slog.New(slog.NewJSONHandler(os.Stdout, nil))
json.Info("hello", "count", 3)
// {"time":"2022-11-08T15:28:26.000000000-05:00","level":"INFO","msg":"hello","count":3}But also, though it's not ideal, I still think their default handler may be the best default for human readable logs. And (assuming you haven't made the switch to a fully parseable structured logging formatter) it may also be the best default for machine parsing. I'll explain below. logfmt
The default handler is an "impure" version of logfmt, and the very first logfmt blog post was written (by a Heroku employee?) with a ruby code example (using Sinatra). :) And, although logfmt isn't formally standardized like JSON, it's fairly intuitive, far more human readable than JSON, and is widely used in many languages (including in the The "impure" logfmt is such an intuitive format that many people (myself included) have simply stumbled into using (inconsistently) inside our own custom log messages. The impure format is just a regular log entry with some key=value pairs inside the message. In my experience, the key=value pairs usually make most sense at the end, although the toy examples don't really show that. I'd used the "impure logfmt" format for years before I ever heard of logfmt or structured logging: logger.debug { "ambifacient lunar waneshaft side fumbling: foo=%p bar=%p" % [foo, bar] }For human readable logsThe message is still the most important part of almost all log entries. So, for human readable logs, the message should come after a relatively limited set of standard fixed width "header" fields: e.g: timestamp, pid, component (progname), request ID, IP address, etc. But, without any other application or framework specific knowledge of which context fields deserve prime position (e.g: request id, user id, IP address), message should come before context. Because, without application knowledge, context is dynamic width and has no practical limits. So putting it first could bounce the message around a lot. This is especially true of context that's been inherited through call stack. When I (a human) am scanning log lines, the messages can bounce around a little bit, but I want the messages in most lines to mostly line up, visually. But if I'm tracing log entries produced by a single Different context typesIf your primary use case is rails tags... well, for the most part those are relatively limited, standard, and don't vary widely range in width. We could easily customize the formatter to tell it which fields come before the message. But the default unconfigured formatter won't know which fields those are. This is also another place it it may make sense to distinguish between a logger's default context (where am I?), the fiber-local context (how did I get here?), and local context (extra structured information for specific log messages). It often makes sense to put the first two types before the message. Only rarely would I want the last type before the message. But it's always "safest" to put all three after the message. So, like go's ExamplesRegardless of everything I just wrote, "human readable" is admittedly very subjective. Nevertheless, I believe you'll see what I'm talking about with some examples. Please compare the following: a big mess, barely readable, IMO. the messages are still bopping about from line to line, but at least I can read the timestamps, PID, and program name now! This is what I proposed. It may not be ideal, but at least I can read the messages without getting lost scanning each line. For machine parseable logsStrictly speaking, it doesn't matter where pure structured logging formats like logfmt or JSON logs put the message. But, let's assume you haven't made the switch to pure logfmt or JSON, for one reason or another. Where should the context go by default for machine readability? For context (about my personal interest in the feature), I use
Yes, in the toy example it's ambiguous... but I'd guess that the ambiguity very rarely matters. Because, in practice, the So, the way logfmt parsers usually1 work is that:
This means that logfmt parsers can generally handle "impure" log entries just fine. They can parse Theoretically, the parser could work around that, e.g: maybe bare word tags won't override key=value pairs. But parsing impure entries isn't the parser's priority, and those heuristics might be wrong... they can't know. None of the parsers I looked at bothered with any workarounds. So, putting the context last is "safest" from a machine readability perspective. This means that, if you're not going to encapsulate all of the fields (including the message) in logfmt, it's safest to put the logfmt fields last, so they can override any accidental "tag fields" that are parsed from the message. Like I said, I do use
I don't know. Probably. But there are practical reasons, both for humans and parsers, so I'm maybe they had similar rationale to mine? 🤷🏻 An updated proposalAnyway, with all of that said: I partially agree with you. In the examples we've been looking at, tacking the context on at the end does seem a bit weird to read. It doesn't really help that it's a better default... it's still not great. But, since I took the time to read through the logfmt parsers and how they work, I now know how they handle "garbage". So I now feel comfortable saying we could keep the logfmt parsers happy while also being more human readable. Here's a small modification to my proposal: Yes, the space before the closing bracket is necessary. 😦 Half of the parsers I looked at will treat the brackets as atom characters. What do you think? Would something similar make sense to you? Footnotes
|
|
@nevans thx for the extra info, I never used logfmt before, so I stand corrected. About the default log format you propose, I agree with you that, when it comes to reading, it definitely makes it more pleasant. Honestly, my whole point for going with that format for tags was more about be less surprising than personal preference. And most rubyists will experience logs through the lens of the rails logger. And looking at how other logging libraries format context/tags by default, such as I'm also thinking that the main goal of this proposal is to enable per-scope/call context, more than how things should be formatted. So for now, I'll lean on the conservative side and keep the proposal as-is (unless you convince the maintainer of the gem, at which point I'll happily change it). |
|
Sorry. That last comment was huge... and yet I have another large addendum: Because I hate that closing brace in my updated proposal (just aesthetically 😉)! So, how about this? And, since the rails tags style seems to be a big part of your inspiration/motivation, and I do use rails style log tags myself (quite a bit!), I think it does make sense to support that explicitly, yeah? Although I do feel that enforcing "context is always hash" is a good simplifying default (and one that subclasses can always override), treating Also, currently we can customize the formatter's datetime format. Why not also add the ability to customize which fields are placed before the message, and how they printed? logger.formatter.prefix_context_fields = %i[request_id ip_address user_id]
logger.info "my message", context: {foo: "bar", user_id: 123, request_id: "some-uuid"}
# => I, [2025-10-02T18:00:31.424331 #3705893] INFO -- request_id=some-uuid user_id=123 : this is my message [Context]: foo=barAnd, while the bracketed tag approach is less machine parseable, it makes a lot of sense for human readable logs. So why not add the ability to customize how which fields are treated as tags, and how those tags appear? # configure how "extra context" (not configured as a prefix) is delimited
logger.formatter.context_format = " %s" # the simplest form
logger.formatter.context_format = " [context: %s ]" # my updated proposal yesterday
logger.formatter.context_format = " [Context]: %s" # my proposal above, in this comment
logger.formatter.context_format = " ~~~ %s" # used by my examples, below
# context for all of the following examples
context = {
foo: "bar",
user_id: 123, # we want to print this as a prefix, keeping "user="
request_id: "some-uuid", # we want to print this as a prefix tag, without "request_id="
component: "settings", # we want to print this as a prefix tag, without "component="
tags: %i[foo bar baz], # we want to treat this as a list of tags
admin_user: true, # always printed as a tag when value is true
}
logger.with_context(**context) do
# with no extra customization
logger.info "this is my message"
# => I, [2025-10-02T18:00:31.424331 #3705893] INFO -- : this is my message ~~~ foo=bar user_id=123 request_id=some-uuid component=settings tags="[\"foo\", \"bar\", \"baz\"]" admin_user
# Configure fields to be extracted from context and printed first, in a specific order.
logger.formatter.prefix_context_fields = %i[request_id ip_address component user_id tags]
# To delimit from the message, print the prefix context between 'progname' and ':'.
logger.info "this is my message"
# => I, [2025-10-02T18:00:31.424331 #3705893] INFO -- request_id=some-uuid component=settings user_id=123 tags="[\"foo\", \"bar\", \"baz\"]" : this is my message [Context]: foo=bar admin_user
# configure some array field values to be handled as an array of tags
logger.formatter.prefix_tag_fields = %[tags] # maybe this should be the default
logger.info "this is my message"
# => I, [2025-10-02T18:00:31.424331 #3705893] INFO -- request_id=some-uuid component=settings user_id=123 foo bar baz : this is my message ~~~ foo=bar admin_user
# convert some some string field values to be printed as tags
logger.formatter.prefix_tag_fields += %i[request_id ip_address component]
logger.info "this is my message"
# => I, [2025-10-02T18:00:31.424331 #3705893] INFO -- some-uuid settings user_id=123 foo bar baz : this is my message ~~~ foo=bar admin_user
# Add brackets to prefix context fields (for readability)
logger.formatter.bracket_prefix_context_fields = array # specific prefix_content_fields
logger.formatter.bracket_prefix_context_fields = :tags # only "tag" prefix_content_fields
logger.formatter.bracket_prefix_context_fields = true # all prefix_content_fields
logger.info "this is my message"
# => I, [2025-10-02T18:00:31.424331 #3705893] INFO -- [some-uuid] [settings] [user_id=123] [foo] [bar] [baz] : this is my message ~~~ foo=bar admin_userWith a few other minor tweaks, you basically have the format used by ActiveSupport. BUT, with all of that said, I think the first PR should be as simple as possible. So, none of the above. IMO, the above should be one or more follow up PRs, so formatter enhancements can be discussed separately. And, printing the context last (maybe with a delimiter) is, IMO, the simplest thing. |
|
Following up on this message, @sonots the gem maintainer hasn't provided feedback in the 3 months since I opened the PR. @jeremyevans @byroot @nobu tagging you as core maintainer who have provided feedback, can I ask you to bring this subject to the core team and find a path forward? As a sidenote, I wrote a blog post which provides more context about motivation / state of the art that otherwise wouldn't fit in a PR comment, hope it's useful. |
|
The process to get this sort of features discussed is to add the ticket to the next developer meeting: https://bugs.ruby-lang.org/issues/21689 Have you done that already? That being said, it's mostly for ruby-core, gems it kinda depends. As for my personal feedback, IMO a library like logger shouldn't have to know about Fiber / Thread etc. If this was me I'd just go with a generic "context store" API, and it'd be the user responsability to provide the right context store implementation for the execution environment. |
|
@byroot thx, wasn't aware of the process, already added. I don't mind the idea of providing context store as an argument, as long as the logger lib ships with the implementations for thread-safe/fiber-safe/fiber-safe-with-inheritance, to avoid having same-but-different implementations spread across. |
I'd recommend including a bit more of the high level justifications in your bullet points. |
|
thx for the suggestion, updated. |
Proto implementation of the proposal from #131, no tests and scoped to
#infoonly for discussion purposes.