ruby / ruby/logger

Level/Context override storage

Open
#138 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
161
Forks
71
Avg merge
3d 22h
Merged PRs (30d)
2

Description

I submitted a couple of PRs that solve the basic problems I have with the current level overrides:

  • #136
  • #137

But I'd prefer to change it a bit more.

If #132 (or something like it) is accepted:

  1. 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.
  2. To share storage with a hash-like API, that suggests that level_key and context_key need to have different values. We could use something like Fiber[:logger_level_key] ||= Object.new, to support both Hash and ObjectSpace::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 #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:

  • This allows users to customize context storage implementation without subclassing Logger.
  • By encapsulating all execution state context, we can remove implementation details (like #level_key) from the Logger class.
  • This simplifies using Fiber.storage for inheritance of logger context.
  • This can easily be extended to use the same strategy for both level overrides and context (see e.g: #132).

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read #132, #136, and #137 first, then trace Logger#initialize and the #with_level and #with_context entry points. Compare the proposed ContextStore API with the current override storage and resolve the Fiber inheritance behavior. Done means customizable shared context storage works without subclassing Logger and the existing override behavior remains covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.