Skip to content
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

perf(common): In case of an expensive log, allow to pass a function #11281

Merged
merged 1 commit into from Mar 22, 2023

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Mar 14, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

In certain case, it is very expensive to log some values. Instead of checking if certain level is enabled, allow the users to pass in a function that is only called if we need to stringify the log value.

This keeps the API small, while still allowing expensive logging to be called when needed, and skipped when not.

e.g. instead of

// This check is technically done twice.
if (this.logger.isLevelEnabled('debug')) {
  this.logger.debug(this.generateAsciiArtFromJpeg());
}

the customer could use:

this.logger.debug(() => this.generateAsciiArtFromJpeg());

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

In certain case, it is very expensive to log some values. Instead of
checking if certain level is enabled, allow the users to pass in a
function that is only called if we need to stringify the log value.

This keeps the API small, while still allowing expensive logging to
be called when needed, and skipped when not.

e.g. instead of

```javascript
// This check is technically done twice.
if (this.logger.isLevelEnabled('debug')) {
  this.logger.debug(this.generateAsciiArtFromJpeg());
}
```

the customer could use:

```javascript
this.logger.debug(() => this.generateAsciiArtFromJpeg());
```
@coveralls
Copy link

Pull Request Test Coverage Report for Build 05c156b5-edfc-4006-829e-036c213cca17

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 92.887%

Totals Coverage Status
Change from base Build 4b917bc9-b3ce-4b2b-9b8e-5fabd7d2d2e1: 0.001%
Covered Lines: 6490
Relevant Lines: 6987

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

Hmm this one's odd. Using this.logger.isLevelEnabled('debug') shouldn't be required, all "debug" level logs should be auto-ignored when the "debug" level is turned off 🤔 Do you have a tiny reproduction repository that illustrates where/when you have to use this check explicitly?

@jmcdo29
Copy link
Member

jmcdo29 commented Mar 15, 2023

I can at least speak to the idea of it, I'll try to make a repo for this later if hansl doesn't.

This would be like if someone wanted to optionally print out a value from a function, and the function took a lot of time or resources to complete, but only call the function if in debug mode. Right now, the function would have to be called regardless and that computation time would be taken regardless of the log level.

this.logger.debug(someComputationallyHeavyMethod())

To get this to optionally print and not take up those computation cycles, right now we'd have to do this this.logger.isLEvelEnabled() check manually, which the logger will then do again during the logging process. What this PR does is allow devs to pass in a function that will be evaluated after checking if the level is enabled so that those computation cycles don't get taken up when the level is not enabled.

this.logger.debug(() => someComputationallyHeavyMethod())

In this case, the heavy computation method would only be evaluated, and its result printed, when the debug level is enabled. Under the hood, the logger would still do the this.isLevelEnabled('debug') check, and then would get the value from the passed function before passing it on to the write method. Hopefully that made some sense, I'll work on an example repository as well though

@hansl
Copy link
Contributor Author

hansl commented Mar 15, 2023

My specific use case was that I have some large ArrayBuffer that I want to debug in hexadecimal (think multi kb requests and responses). This process is too heavy to do every time on every requests, and since prod doesn't use debug logging it doesn't need to perform those operations.

@jmcdo29 100% described it better than I did in the PR description. Thanks!

@kamilmysliwiec
Copy link
Member

That makes a lot of sense @jmcdo29 @hansl, thanks for the clarification 🙏

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants