slog icon indicating copy to clipboard operation
slog copied to clipboard

Warn agains wrapping Logger in Logger::root in docs

Open CodeSandwich opened this issue 5 years ago • 2 comments

Context

As a user I have a Logger and I want to create a new Logger, which is the same as the old one, but has additional key-value.

Problem

I see that Logger implements Drain, so I can wrap it in a new Logger the same way I created it: with Logger::root.

Wrong! Logger's implementation of Drain involves allocating an Arc on every log. For the special case of wrapping Logger I should use Logger::new, which has no such performance penalty. Even if I find out that Logger::new exists, I still have no idea, which route should I take.

Solution

It would be nice if Logger::root and Logger::root_typed docs explicitly warned against wrapping another Logger and pointed to Logger::new.

CodeSandwich avatar May 23 '19 12:05 CodeSandwich

I think I've just completely missed the fact that impl Drain for Logger is bad perf-wise. Maybe we should just deprecate the whole thing?

dpc avatar May 23 '19 18:05 dpc

I think that impl Drain for Logger is a very nice closure of idea of Drains and in some cases it might add crucial elasticity. The depreciation could affect legitimate uses with flood of warnings and at some point enforce creation of cumbersome wrappers.

I doubt that impl Drain for Logger could have no overhead either.

CodeSandwich avatar May 23 '19 21:05 CodeSandwich