Skip to content
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

Add logger configuration for backends #1294

Open
bkuhlmann opened this issue Feb 28, 2023 · 12 comments
Open

Add logger configuration for backends #1294

bkuhlmann opened this issue Feb 28, 2023 · 12 comments

Comments

@bkuhlmann
Copy link

bkuhlmann commented Feb 28, 2023

Overview

Hello. 👋 I ran into a situation where I needed to colorize logging in my development environment while also logging to a file and was wondering if it'd be possible to add config.logger.add_backend support instead of config.logger.instance.add_backend?

I realize this is a minor quality of life improvement in terms of intuiting matching functionality as found in Dry Logger but would reduce a few characters of typing and feels a bit nicer. Also, having to mutate the instance feels wrong too.

Steps to Recreate

Here's how to make this work:

# config/app.rb
environment :development do
  config.logger.options[:colorize] = true

  config.logger = config.logger.instance.add_backend(
    colorize: false,
    stream: Hanami.app.root.join("log/development.log")
  )
end

Environment

  • Ruby: ruby 3.2.1 (2023-02-08 revision 31819e82c8) +YJIT [arm64-darwin22.3.0]
  • Hanami: 2.0.3
bkuhlmann added a commit to bkuhlmann/hanamismith that referenced this issue Feb 28, 2023
Necessary to provide colorized logging within the console for improved readablity while also logging to a file for historical context (if needed). I've logged an [issue](hanami/hanami#1294) for consideration that could make this less tedious.
@solnic
Copy link
Member

solnic commented Mar 1, 2023

Yeah I'd be in favor of doing this even though it'd make things a bit more complicated on the config side. We could even capture all method calls that are supported by the instance and eventually forward them once the instance is available.

@bkuhlmann
Copy link
Author

bkuhlmann commented Mar 2, 2023

Yeah, this would be great because if the Hanami configuration for the logger is a simple proxy it means you could potentially do something like this where you chain the desired behavior like you can do with the native Dry Logger (well, sort of, I'm dreaming a bit here). Example:

config.logger
      .colorize
      .formatter(:rack)
      .add_backend(stream: Hanami.app.root.join("log/development.log")

This would make configuring the logger rather elegant but probably the most powerful aspect of this is being able to add multiple backends easily with minimal effort.

@solnic
Copy link
Member

solnic commented Mar 2, 2023

Right. My only concern is that config object methods may potentially clash with logger methods. I wonder now if maybe using a provider for logger to tune the instance would be better? Something like:

class MyApp < Hanami::App
  # This doesn't exist, but we could potentially have shortcuts like that
  prepare(:logger) do
    logger.add_backend(stream: ...)
  end
end

The reasoning here is that extending config with even more options just to translate them into method calls is actually more complex than exposing a convenient way to access components (logger in this case) in order to tweak them using their own API.

WDYT @jodosha @timriley?

@timriley
Copy link
Member

timriley commented Mar 2, 2023

FWIW, right now you can actually provide a complete replacement for the logger by creating your own provider at config/providers/logger.rb. Hanami::App will check for this provider before attempting to register its own for the built-in logger. I know this is even more boilerplate than what prompted @bkuhlmann to file this issue, but I thought I'd share this just in case :)

The logger is one of our more complex bits of config, in that we want to be able to allow users to tweak specific bits of logger config without having to replace it wholesale (so they can leverage the framework's defaults where possible), while at the same time allowing that logger to be wholesale replaced if the user so desires.

The much more powerful and dry-logger integration also came into Hanami right before our 2.0 release, so we haven't really had much time to step back and think this through again. Your input here is helpful in this regard, @bkuhlmann.

One thing I do know is that I'd rather not add new or special general purpose APIs (like you've suggested here with this App-level prepare idea above, @solnic) — providers are already going to be a new thing for folks to learn and I think we'd do well to make sure they're all used in a consistent way.

What I do think would be helpful here is to continue to take a "wait and see" approach. Will many others come to us with suggestions like @bkuhlmann's here? We don't know yet! And I'd like to make sure we're not putting extra work or complexity into areas that don't really need it.

Because, let's be honest, this snippet of code from the original post isn't too bad at all, really:

environment :development do
  config.logger.options[:colorize] = true

  config.logger = config.logger.instance.add_backend(
    colorize: false,
    stream: Hanami.app.root.join("log/development.log")
  )
end

It's still pretty understandable, and the fact that this kind of advanced arrangement is possible is already a win :)

Granted, it is kind of awkward that the logger has to be reassigned like that, so if anything, perhaps we can just smooth over this part in particular, something like:

config.logger.prepare do |logger|
  logger.add_backend(
    colorize: false,
    stream: Hanami.app.root.join("log/development.log")
  )
end

This block would be called with the logger at the end of Config::Logger#instance.

@bkuhlmann
Copy link
Author

Peter/Tim:

  • Funny that you both mention using a custom logger provider. I started down that route, initially, then thought maybe using the app configuration was a bit more elegant because I could leverage the environment block more easily. 😉
  • I think using a custom logger provider makes sense but how do you suggest dealing with environments? At least with the application configuration, I can use the environment block. ...but at the provider level, I'd have to use if or case statements in order to configure the logger for different environments. Probably not bad but there is a nice elegance to using the environment block.

Tim:

  • I might be missing what you are suggesting -- also I hadn't noticed logger provider check so thanks for pointing that out 🙇 -- but if I create a Hanami.app.register_provider :logger { # Implementation }, I'll end up with a key not found: "logger" (Dry::Core::Container::KeyError.
  • Alternatively, I can use Hanami.app.register_provider :logger_configuration { # Implementation } but use of logger_configuration as the provider name feels awkward and doesn't immediately prepare the logger when I jump into the Hanami console to log a message versus using the environment configuration in my code snippet at the start of this discussion. I was hastily testing this theory so probably doing something silly on my end. 😅

On final thought, Tim, because I think this is interesting (Peter hints at this as well in terms of simply exposing the logger instance for further tweaking):

The logger is one of our more complex bits of config, in that we want to be able to allow users to tweak specific bits of logger config without having to replace it wholesale (so they can leverage the framework's defaults where possible), while at the same time allowing that logger to be wholesale replaced if the user so desires.

I definitely don't want to make any of this more complicated but feels like a potential solution falls within the following criteria (maybe?):

  • Be able to globally configure the logger with safe defaults.
  • Allow the logger -- or any object for that matter -- to be exposed so you can interact/modify it (per Peter's suggestion).
  • Allow any provider to interact with the default logger instance while also being able to potentially leverage a environment block for further customization while using minimal syntax.

@solnic
Copy link
Member

solnic commented Mar 3, 2023

Thanks Tim for chiming in ❤️

I think using a custom logger provider makes sense but how do you suggest dealing with environments? At least with the application configuration, I can use the environment block

You can always use Hanami.app.environment inside provider blocks but maybe we could provide a shortcut for it so just environment could work too. WDYT?

@timriley
Copy link
Member

timriley commented Mar 3, 2023

Yeah, I like the idea of an environment helper inside providers! It's not uncommon that they may need to do different things per env, and we should make this feel nice :)

@timriley
Copy link
Member

timriley commented Mar 3, 2023

Implementation-wise, that will require more hooks into dry-system, however. Right now dry-system entirely "owns" the provider lifecycle-related code.

We'd need to update dry-system to accept a custom provider source class in its config (or something like that, I haven't thought about it too deeply). This would allow Hanami to configure it with a custom subclass that provides this environment helper.

In fact, there's one more related thing I've wanted to do in providers: right now, target in a provider refers directly to the Dry::System::Container class. When providers are used in Hanami, however, I'd love it if target could return the Hanami app or slice class instead. This would make things feel much more cohesive.

Honestly, however, I'd prioritize all of these much lower than finishing our view and persistence layer. These would be really nice bits of polish for when we do get to them though. Sometime this year!

@bkuhlmann
Copy link
Author

Peter: Thanks for the Hanami.app.environment tip, I'll use that as workaround until a potential helper is introduced in the future. 🙇

Tim: Yep, more persistence and view layer support first would be most welcome. 🙇 By the way, I've been struggling with the use of target terminology. I know you can access container within a provider but both target and container are containers. It feels like target should be renamed as container but then not sure what to do with Dry::Core::Container as the remainder. Anyway, some thoughts and definitely out of scope for this issue. I can log this as a separate issue if that'd be of interest/help?

@solnic
Copy link
Member

solnic commented Mar 6, 2023

By the way, I've been struggling with the use of target terminology

Thanks for bringing this up, but let's not have this convo here as it's not strictly related to the logger issue. Could you start a thread in the discussion forum? I'm also not a fan of target and would love to hear your thoughts/ideas how this could be renamed 🙂

@timriley
Copy link
Member

timriley commented Mar 6, 2023

This is more a dry-system naming issue than Hanami, FWIW.

If we can have Hanami provide its own dry-system provider source implementation (per my thoughts above), then we can just make it named "slice" (and additionally app when we know the provider is being registered with the app) as an alias of target.

(BTW, target itself is an alias of target_container, just as container in the same context is an alias of provider_container. Those names might be more self-explanatory, albeit longer)

There's are good reasons these two different containers exist for each provider, and I just need to fine the time to write it down 😩. For example, we need them to support providers registered with a namespace. And they open up a range of interesting use cases where external provider sources are used and then the user of that provider source can more exactly control what eventually gets registered in the target container. FWIW, the multiple containers were not actually my invention! So this is definitely not me here trying to defend my "baby." In fact, during my big dry-system refactors of 2021/2022 I tried to remove the provider container, and only in doing that did I realise its usefulness.

@bkuhlmann
Copy link
Author

bkuhlmann commented Mar 6, 2023

Peter/Tim: OK, I extracted this discussion to this discussion proposal to keep this issue clean. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants