Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: do not stringify headers for logging #61

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

achingbrain
Copy link
Collaborator

The frame headers are passed through the stringifyHeader for trace logging if a logger exists. Unfortunately a logger is set by default so this always occurs.

If we need to log these things as a string, checking that the logger is enabled is one option:

if (this.log.enabled) {
  // do expensive logging operation
}

Another is adding a custom formatter to the log class, or just having the header object appear in the console.

The frame headers are passed through the `stringifyHeader` for trace
logging if a logger exists.  Unfortunately a logger is set by default
so this always occurs.

If we need to log these things as a string, checking that the logger
is enabled is one option:

```js
if (this.log.enabled) {
  // do expensive logging operation
}
```

Another is adding a custom formatter to the log class, or just having
the header object appear in the console.
@achingbrain achingbrain requested a review from a team as a code owner November 11, 2023 17:36
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Files Coverage Δ
src/encode.ts 100.00% <100.00%> (ø)
test/codec.spec.ts 100.00% <100.00%> (ø)
test/compliance.spec.ts 100.00% <100.00%> (ø)
test/decode.spec.ts 96.01% <100.00%> (-0.02%) ⬇️
test/muxer.spec.ts 100.00% <100.00%> (ø)
test/stream.spec.ts 100.00% <100.00%> (+0.90%) ⬆️
test/util.ts 100.00% <100.00%> (ø)
src/decode.ts 93.75% <88.88%> (-1.39%) ⬇️
src/stream.ts 98.01% <99.08%> (+2.52%) ⬆️
test/codec.util.ts 94.28% <66.66%> (ø)
... and 2 more

📢 Thoughts on this report? Let us know!

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, in the past, stringifying the header was a performance issue

@wemeetagain wemeetagain merged commit 59e73d8 into master Nov 12, 2023
@wemeetagain wemeetagain deleted the perf/do-not-stringify-headers branch November 12, 2023 06:02
github-actions bot pushed a commit that referenced this pull request Nov 12, 2023
## [5.0.1](v5.0.0...v5.0.1) (2023-11-12)

### Bug Fixes

* do not stringify headers for logging ([#61](#61)) ([59e73d8](59e73d8))
* silence max listeners exceeded warning ([#62](#62)) ([cce9446](cce9446))
Copy link

🎉 This PR is included in version 5.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants