Added isLevelEnabled(string) & isXXXEnabled()
#1352
Conversation
|
Hello and thanks for this logging library! This PR is a proposal and requires some cleanup - tests could be improved and jsdocs are missing. Please let me know if this is even a valid approach and useful feature in general. |
|
Thanks for submitting this PR. This was a previously requested feature, I don't see any strong reason why this won't land soon. |
|
@indexzero do you think implementation in this PR is acceptable? does it require any adjustments? |
|
@lutovich looks like I made some changes in another PR that caused git conflicts here. Would you mind resolving them? Then we should be good to go for merging this 😸 |
Make logger expose functions to check if a particular level is enabled. This could be useful if either message or parameter creation is an expensive operation, which should be avoided when log level is disabled. Logger will contain `isLevelEnabled(level: string)` function and multiple `isXXXEnabled()` where "XXX" is the capitalized name of the configured log level. E.g. `isDebugEnabled()`, `isInfoEnabled()`, etc.
lutovich
force-pushed
the
is-level-enabled
branch
from
16200ac
to
5a112df
Compare
|
@indexzero PR is now rebased :) |
|
Would be awesome to merge and release this :) |
|
In Winston 1.X I remember the logger would do this kind of check up front and therefore not process the second arg (the meta or whatever it's called). We noticed performance decrease in certain cases after we moved to Winston 2.X because this was no longer the case. Any change this request can go one step future and embed the logic in the logger.log() method? We actually wrapped our Winston logger in a Proxy to do exactly this. |
|
Just merged this – it will go out in the next release, which should hopefully be published very soon. I think the comment above is describing a separate issue about the behavior of the log function. If you see any places where we are not correctly optimizing for the hot paths, especially in the log function, on master, please feel free to open a separate issue or PR so we can take a look. Thanks! |
|
@DABH thank you for all you do! And yes, in the off chance I get some spare time I'll try to put together something formal. 🎩 |
Make logger expose functions to check if a particular level is enabled. This could be useful if either message or parameter creation is an expensive operation, which should be avoided when log level is disabled. Logger will contain `isLevelEnabled(level: string)` function and multiple `isXXXEnabled()` where "XXX" is the capitalized name of the configured log level. E.g. `isDebugEnabled()`, `isInfoEnabled()`, etc.
|
Did this ever get documented anywhere though? I can only find mention of it in GitHub threads. This is a very important feature for efficient log practices at high scale and should be in the docs I would say! |
|
@peternann If you don’t spot anything relevant in the docs, I’m sure other devs have similar feelings :) A PR is welcomed! |
Make logger expose functions to check if a particular level is enabled. This could be useful if either message or parameter creation is an expensive operation, which should be avoided when log level is disabled.
Logger will contain
isLevelEnabled(level: string)function and multipleisXXXEnabled()where "XXX" is the capitalized name of the configured log level. E.g.isDebugEnabled(),isInfoEnabled(), etc.Resolves #905