pimterry / loglevel

:ledger: Minimal lightweight logging for JavaScript, adding reliable log level methods to wrap any available console.log methods
MIT License
2.62k stars 157 forks source link

Configurable persistence #165

Open xmedeko opened 3 years ago

xmedeko commented 3 years ago

getPersistedLevel is called for each logger. I do not need that. I think the persistence should be configurable same way as methodFactory for plugins - i.e. it should be possible to switch it off, use only cookies, etc.

pimterry commented 3 years ago

Hi @xmedeko. Could you tell me more about why you need to disable this?

Right now, persistence is fully configurable at level configuration time - you can pass an option to setLevel to control this. If you never call that with persistence enabled, then nothing will ever be stored, and getPersistedLevel will never do anything, and has no impact. Is there a reason that doesn't work for you?

Unfortunately, if it is a problem, it would be difficult to disable persistence at load time. By design loglevel is immediately initialized and available when imported, with zero options or construction required, to make it as easy as possible to quickly use. That means that getPersistedLevel is already called before the point where you could configure it.

xmedeko commented 3 years ago

I would like to disable it because it's useless for our Electron.js app. It's a very small slow down for every getLogger() call (and I use getLogger() often.)

May be you think - hey, the slow down is negligible, why do you care? My reply is: why not to care? Why to have some code reading localStore and cookie which I do not need? IMO a good logging library should do just logging and everything else should be optional (or when default, then possible to opt-out.) And loglevel is almost like that except the persistence 😀

I understand it's not possible to change it for the current API. So I suggest:

pimterry commented 3 years ago

It's not called every time you call getLogger, it's called once for each different logger that you use. If you pass the same name twice, it's only called the first time around: https://github.com/pimterry/loglevel/blob/342f103eed48f29db854533d2cbfe254690f9113/lib/loglevel.js#L247-L252

I've done a little digging, and as far as I can tell in practice you'd have to instantiate many thousands of distinct logger instances before the local storage check has even 1ms total impact. If you're doing that much logging frequently enough that this has a measurable impact (e.g. creating 1000 loggers every second, each with distinct keys) then it's likely that you're generating huge amounts of logs, and that will be significantly more expensive than any localstorage lookups.

That said, if you're committed to trying to avoid this regardless:

Honestly though, I don't think you should do any of this. LocalStorage is only expensive in comparison to bare object property lookups, loglevel is not optimized for performance to that degree at all, and unless you're working in some very specific high-performance environments the rest of your JS application is not either. There will be other much easier and more effective performance improvements in your code elsewhere!

If you are doing a quantity of logging where this truly matters, and you're really really sure that this practically affects your performance, imo you're better off rolling your own microlibrary entirely. This is a tiny lib and it has lots of small UX & cross-browser compatibility details you could skip entirely for a custom case. There's probably many other micro-optimizations and simplifications here that would save >1ms in such an application, e.g. if you're calling getLogger billions of times then I suspect removing the argument validation checks in getLogger that happen on every call would make a bigger practical difference (but changes like that doesn't make sense for loglevel itself, since there's a UX tradeoff).

xmedeko commented 3 years ago

I just repeat my previous comment:

May be you think - hey, the slow down is negligible, why do you care? My reply is: why not to care? Why to have some code reading localStore and cookie which I do not need?

Instead of getLogger({ persistence: false }) I would welcome some configuration like methodFactory - e.g.:

require('loglevel').persistence = {
    getLevel(loggerName) { ... },
    setLevel(loggerName, levelName) { ... }
}

Which would be more versatile.

This is not a blocker for me, just IMO such things should be configurable. And, of course, I can always do copy&paste a remove that code.