-
Notifications
You must be signed in to change notification settings - Fork 69
Description
I submitted a couple of PRs that solve the basic problems I have with the current level overrides:
But I'd prefer to change it a bit more.
If #132 (or something like it) is accepted:
- I believe that the "fiber scoped" block overrides for both
#with_level
and#with_context
should use the same storage strategy. Rather than keeping two separate maps, I think it's better to combine them into a shared strategy:@override_store
or@context_store
. - To share storage with a hash-like API, that suggests that
level_key
andcontext_key
need to have different values. We could use something likeFiber[:logger_level_key] ||= Object.new
, to support bothHash
andObjectSpace::WeakKeyMap
, but that's kinda gross.
The current approach doesn't allow us to cleanly swap in the Fiber-storage API:
- Fiber doesn't implement
#delete
- We can't reset storage with
clone.clear
(which is what I used in Clear level overrides in#dup
and#clone
#137).
Fiber storage does face the additional question of how to handle when fibers aren't nested in a "structured concurrency" manner: does the child continue with its inherited storage after the parent leaves the #with_level
or #with_context
block. I think yes, it should... but others may want different behavior.
And many applications, frameworks, or libraries will already have their own encapsulation of execution context, which may not be identical to Fiber.current. As an example of this, see ActiveSupport::IsolatedExecutionState
, which is used by the ActiveSupport::LoggerThreadSafeLevel
mixin.
All these concerns can be addressed by creating a simple API, creating a default class that implements that API, and allowing an instance to be passed in as an argument to #initialize
.
I propose something like the following:
class Logger
class ContextStore
def initialize
@levels = ObjectSpace::WeakKeyMap.new
end
def level_override = @levels[Fiber.current]
def level_override=(val) @levels[Fiber.current] = val end
def clear = @levels.clear
private
def initialize_copy(original)
super
@levels = @levels.dup
end
end
def initialize(*, context_store: nil, **)
# ...
@context_store = context_store || ContextStore.new
# ...
end
end
This mimics the current implementation, with the addition of #136 and #137.
The benefits of this approach:
- by encapsulating all execution state context, we can remove implementation details (like
#level_key
) from theLogger
class. - it allows users to custom context storage without subclassing logger
- it can easily be extended to use the same strategy for both level overrides and context