(perf) Refactor logger class to encapsulate bunyan instance; add requestLogger stress test #68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
January 18, 2017 19:44
mryave
approved these changes
Jan 23, 2017
There was a problem hiding this comment.
I think we should all read this. Its pretty awesome. @GLosch @ricardo-quinones
| */ | ||
| export default function ClientLogger(config = {}) { | ||
| export default function ClientLogger(config = {}, logger) { |
| requestsPerSecond: 20, | ||
| }; | ||
|
|
||
| function printResults(results) { |
mattjstar
reviewed
Jan 23, 2017
| const newArgs = args.map((arg) => scrub(arg, this.config)); | ||
| return original.apply(this, newArgs); | ||
| } | ||
| // Dynamically hoist + wrap bunyan log instance methods (logger.info, logger.warn, etc) |
There was a problem hiding this comment.
might be worth adding to readme that we're only hoisting these logger levels, if we want to access the api you have to logger._logger.addStream...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is a fun one. We've been seeing some performance issues with this logger since the
scrubFieldsfunction was added. Related to #47 #53 #54 #58This change updates the
Loggerclass to encapsulate a bunyan logger instance rather than return one. This allows us to be much more explicit with the behavior we'd like the logger to maintain.It also changes the behavior of or data scrubber -- the entire
argumentsobject is passed tohide-secretsnow (only ifscrubFieldsis configured), which avoids some bottlenecks in this path.Benchmarks
The slowdown from scrub fields is significant, but also expected. I think this is still in the realm of "acceptable" performance since we're not logging anywhere near 30-40k times/sec in our apps.
The
childmethod is noticeably slow. This is a bottleneck that we should look into optimizing (#67), but it should be ok for now.Stress tests:
A stress test suite was added in this PR that spins up a simple
expressserver with therequestLoggermiddleware and puts it under load. Here's what that looks like as of this change:These tests can be run via
npm run test:stress