Skip to content

Convert to "debug" library for debug logging #231

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

Closed
wants to merge 1 commit into from

Conversation

yinzara
Copy link

@yinzara yinzara commented Oct 28, 2020

The current implementation of logging in json-rules-engine is extremely inefficient when debug logging is disabled (the most common [and default] case). Each debug statement constructs a new string of the debug message then checks a set of environment variables before ignoring the string all together.

The "DEBUG" environment variable when set to "json-rules-engine" enables debug logging across the entire library and makes it difficult to know which file the messages are coming from.

This PR adds the extremely popular and lightweight debug library and converts all existing "debug" statements to the placeholder based syntax supported by "debug". Now when debugging is not enabled (the most common and default case), debug messages do not construct new strings and no additional overhead is added. If debug is enabled, the strings are now constructed using the standard debug formatters (and one additional "typeof o" formatter added in debug.js)

"debug" supports both NodeJS runtimes and browsers and can be enabled using the same DEBUG environment variable. See the library link above for more information.

@CacheControl
Copy link
Owner

CacheControl commented Oct 29, 2020

Hi @yinzara, I'm generally a fan of the debug library and use it in many of my other projects. In this case however, I actually had to remove it (#125, #130) because of breaking changes that were introduced in v3 and v4 which caused ES5 compatibility problems reported by multiple users.

@yinzara
Copy link
Author

yinzara commented Oct 29, 2020

Hmmm I did not realize that it had been used and removed previously. While I agree the issue of a dependency causing downstream issues is a problem, the solution that was made is not appropriate in my mind.

I don't mind removing debug and writing a more custom debug solution, however I don't think constructing strings and then checking an environment variable with every call to log is an appropriate solution either. I actually kinda liked the solution of #125 of moving it to dev dependencies and then failing if it doesn't load and just ignoring. However if you'd prefer I can write a simple logger function that can handle the placeholders. You could also simply lock the debug library to an earlier version but I don't generally like doing that.

I also think that the environment variables should be read at application start and not with every log message. That does mean a user who is debugging needs to restart the app to change the logging status, but I think that's a small price to pay to reduce the processing while the app is running for the vast majority of the cases as of right now it's checking environment variables constantly.

If you really want to keep the string log messages, if you made your debug function accept a "() => string" instead of a "string" at least we could only construct the string when we're actually going to print the message. Right now it's literally exactly what we get told not to do in intro to programming class 25 years ago.

@yinzara
Copy link
Author

yinzara commented Nov 20, 2020

@CacheControl Any thoughts on my prior comment?

@CacheControl
Copy link
Owner

Hi @yinzara - your points really seem to be focused on the perception that this code is causing a performance problem. If you're able to demonstrate this, I'm happy to look at some of the optimizations you've suggested. However, I've benchmarked this library extensively, and this was not identified as an area of concern. Based on my most recent analysis, I'd have to go through a very long list of other optimizations before this particular section became a measurable concern. So although I agree it's inefficient, it's not inefficient enough to warrant attention

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

Successfully merging this pull request may close these issues.

2 participants