Add an optional logger param

[tisonkun]

This closes #645

@KodrAus I noticed that we previously introduced target for similar usage, but switching to a different logger is different.

This patch is for preview. I'm not quite sure how we pass the customized logger. The current implementation seems good. I think the log crate itself isn't suitable to hold a global name -> logger registry: that would be too heavy and too specific.

[tisonkun]

This is just a good target to test our new logger param.

Previously, the global logger state is shared with all the macros tests and causes the result unstable.

[Thomasdezeeuw]

I'm not convinced this is a common enough use case that it has to be in facade, couldn't it be implemented in the logging implementation?

[tisonkun]

@Thomasdezeeuw

spdlog-rs implements this, with their own log! macro. When adapting to the log crate's macros, the functionality can't be used.

So, the logging implementation must implement the same functionality by bypassing our macro, which loses the value of a facade. If you have an idea how to implement and use it with our abstraction and a custom impl crate, I'm glad to know.

[Thomasdezeeuw]

So, the logging implementation must implement the same functionality by bypassing our macro, which loses the value of a facade. If you have an idea how to implement and use it with our abstraction and a custom impl crate, I'm glad to know.

I my crate I used the target to change the target of the logs. In this it uses a constant for the target to switch from logging to standard error to standard out. See https://docs.rs/std-logger/latest/std_logger/macro.request.html for an example, but any macro can be used by setting the target to a constant (https://docs.rs/std-logger/latest/std_logger/constant.REQUEST_TARGET.html)

[tisonkun]

Thanks for your information! I'm considering the difference between target and a logger field also.

In the description:

I noticed that we previously introduced target for similar usage, but switching to a different logger is different.

Switching targets still use the same (global & singleton) logger instance, while setting a new logger instance can avoid meta logging cycle - #645.

[Thomasdezeeuw]

Switching targets still use the same (global & singleton) logger instance, while setting a new logger instance can avoid meta logging cycle - #645.

The global logging implementation can be set up to call different loggers depending on target.

[KodrAus]

Personally, I think being able to target a different logger entirely using an argument is useful in a few scenarios:

1. You want to log from your logging pipeline. If your pipeline is complex enough, you sometimes end up needing to do this.
2. You don't want to use a global logger at all, but still want some of the nice ergonomics of the macros. You may do this, for example, to implement your own simplified contextual logging.

From a maintenance perspective it also keeps us a bit honest in not being able to assume that everything is shared by a single global logger.

If @Thomasdezeeuw isn't convinced it's worth adding though then we'll leave it for the time being.

[Thomasdezeeuw]

I'm not convinced the additional complexity is worth the use case that is already possible via the log implementation.

@tisonkun do you have any argument why it should be done this way, and not via the log implementation?

[tisonkun]

It's included in the issue description of #645:

Filter certain targets in specific appended:

impl log::Log KafkaAppender {
  fn log(&self, record: &Record) {
    if record.target().start_with("rdkafka") { return; }
  }
}
```

This is, however, both brittle and may filter logs from user code. I don't know how targets are defined and if we can reliably depend on that matches.

That said, passing a logger instance is determinate for what is installed in the log implementation (pipeline), while target is a loose contract (convention).

If you have a single log instance with a Kafka appender installed, you may first filter out logs with target "kafka". But what if a user application uses the same target for their actions over kafka? Then you need to agree on a reserved target and every user should learn about it.

Instead, a pass-in logger can be an explicit contract that the certain logger is used by kafka client internally (if a client library adopts this) and thus you can fine tune the manner and be sure that it would be used in that scenario.

You don't want to use a global logger at all, but still want some of the nice ergonomics of the macros. You may do this, for example, to implement your own simplified contextual logging.

This is also a good point that when you have different log requirements for different components, instead of making a global instance and dispatching the application logic (rather than barely appending to multiple destinations), it would be easier to keep the logger instance separated.

Finally, if we support logging with a pass-in instance, it's possible to use a global instance everywhere. But the reverse direction is not true. slog's macros always has a logger param, while there is slog-global to use a global instance when it's satisfied.

[NobodyXu]

This PR reminds me of with_default:

Sets this Subscriber as the default for the current thread for the duration of a closure.

The default subscriber is used when creating a new Span or Event.

Sounds like you actually want something like this?

@tisonkun

[tisonkun]

Sounds like you actually want something like this?

@NobodyXu No. This is a thread-local default, which can be even worse than a global default to tune with. The logger field in log! macro is an explicit instance, with_default is a more subtle implict.

[SpriteOvO]

Hi, I'm the author of spdlog-rs, and I'd like to thank @tisonkun for mentioning my crate and nominating to merge this feature into log crate, and @KodrAus for supporting this feature in the issue. I'll give my two cents here.

The introduction of the logger parameter (and replaceable global logger) in spdlog-rs (as well as the same concept in C++ spdlog) was primarily to address the inconvenience of singleton - can only be set once at startup.

Imagine you have a huge project with many components. Some components just print to standard streams, some components need to report logs to a specific network endpoint (maybe even this endpoint needs to be dynamically obtained and unknown at startup). In the singleton model, the initial setup needs to prepare all components for their needs, rather than being managed by the components themselves. And since the discriminator target is just a string, ambiguities may be uncommon but are by no means absent especially when match rules like starts_with are involved.

The addition of the logger parameter is not killing the singleton pattern, which is still the easiest way for lightweight projects, but larger projects can benefit from this harmless addition.

[tisonkun]

Merge master branch and resolve conflicts.

May we have another round to review this patch?

[KodrAus]

I think we'd need to document this behavior. We have a hierarchy of level controls; at compile time, at runtime, and whatever the logger itself applies. We should be clear that these are features of the macros, so apply equally regardless of the logger you plug in.

[tisonkun]

Could you please elaborate a bit what to be documented and where I should add the docs?

It seems that you'd like to emphasize that these macros can be compiled to nop if the related flags (e.g., max_level_error, release_max_level_error, etc.) enabled.

[KodrAus]

This looks good to me @tisonkun 👍

@Thomasdezeeuw are you happy if we move forwards with this feature? I think pushing back on additions is a good position that keeps us from accumulating bloat, but feel like this one specifically is a reasonable enhancement aligned with the existing macro capabilities.

[Thomasdezeeuw]

@Thomasdezeeuw are you happy if we move forwards with this feature? I think pushing back on additions is a good position that keeps us from accumulating bloat, but feel like this one specifically is a reasonable enhancement aligned with the existing macro capabilities.

I still think the use case can be solved without this change, hence I don't think it's worth the additional complexity to be honest.

However, if we go forward with it have we looked at the code size changes? Work has been done in the past to reduce the compiler code size, is that affected at all? /cc @EFanZh in case you're still interested in this

[KodrAus]

I still think the use case can be solved without this change

Some of them can, but some can be done more easily without needing to write dispatching logic based on targets. Being able to plug a logger in to the macro calls would let you use local loggers for contextual logging, without needing a centrally synchronized place to store them. That could also potentially be solved with the key-value support though if we let you plug in an additional Source alongside specific key-values.

Maybe we can think of it more like print! vs write!. The former doesn't let you specify where the output goes, whereas the latter does. If we want to decide that log! is just print! with a configurable output via set_boxed_logger then this change isn't something we'd pursue. If we want the log! macros to be a general interface to any Log impl, then I think it would be very useful.

[Thomasdezeeuw]

I won't block this (any further) if code size isn't a concern.

I do think we should add some more documentation for this change as I don't see any. I would at least expect the logger argument to be documented and stating that by default it will use the global logger.

[tisonkun]

I do this we should add some more documentation for this change as I don't see any. I would at least expect the logger argument to be documented and stating that by default it will use the global logger.

Thanks for your suggestions. Then please leave me a few days so that I can pick up this patch :D

[EFanZh]

The default logger should be a zero sized type, otherwise caller needs pay the cost of passing the logger value.

[tisonkun]

Oh. I get your point now. Let me update it.

[tisonkun]

Please check if this is what you mean 2bb47d6

[SpriteOvO]

What happens if we call a log macro with logger param here? The macro would expand a let logger = $logger;, which seems to move rather than borrow the logger variable.

[tisonkun]

IIUC it would be debug!(logger: &logger, ...) then. That is, use ref from the user size.

One of the rationales is @EFanZh's comment about "binary size", see:

• Add an optional logger param #664 (comment)
• Add an optional logger param #664 (comment)

[tisonkun]

Shall we #[inline] here? In my application development conventions, I trust the compiler to do the inlining based on its analysis.

[tisonkun]

This inspires me to consider whether we should export the NopLogger ..

[EFanZh]

Another thought: with the current implementation, user specified logger will always be passed by reference, which might not be ideal for binary size. Do we want to support the case where user can specify a zero-sized logger to reduce binary size?

[tisonkun]

Another thought: with the current implementation, user specified logger will always be passed by reference

What does it mean? We already accept L that can be an owned logger. Could you give a concrete example where a performance concern can occur.

Does 2bb47d6 implements what you mean for passing a zero-sized logger? If not, why.

[EFanZh]

@tisonkun: Oh, I thought the implementation still passes reference for user specified loggers. Now the implementation passes logger argument as is, then I have no further questions.

[tisonkun]

@EFanZh OK. Good to know the clarification.

Then just leave me some time to figure out how to improve the docs :D