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

Remove some obvious pitfalls. Migrate to winston@3 API #232

Merged
merged 6 commits into from
Apr 6, 2018

Conversation

indexzero
Copy link
Contributor

@indexzero indexzero commented Apr 11, 2017

The changes include:

  • Remove usage of the default logger. TL;DR? Do not use the default logger if you have hard performance needs (like benchmarking).
  • Use a stream Transport to be more closely aligned with feature set offered by peer sets.
  • Begin to move to winston@3 API (notice the level property as a hard requirement for .log)

@mcollina
Copy link
Member

Can you update the package.json so that I can install winston@3?
What do the benchmarks look like?

I'm very very happy about this PR, thanks @indexzero!

@jsumners
Copy link
Member

You say "never use the default logger if you have hard performance needs (like benchmarking)." But Winston's readme does not make this clear, even on the 3.x branch. It merely says "If you would prefer to manage the object lifetime of loggers you are free to instantiate them yourself." That doesn't give the reader any indication that there is a significant performance difference between manual management and using the default logger. Therefore, I assume the way Winston gets used the majority of the time is via the simpler method: the default logger. Am I wrong?

I bring this up because I think our comparison benchmarks should at least include the patterns most generally used by the other loggers. It is certainly desirable to show comparisons against the other loggers's performance configurations as well; indeed, our benchmarks include Pino's "slow" default and fast opt-in mode.

@davidmarkclements
Copy link
Member

let's benchmark both

…needs (like benchmarking). Use a stream Transport to be more closely aligned with feature set offered by peer sets. Begin to move to winston@3 API
@indexzero indexzero changed the title [WIP] Remove some obvious pitfalls. Migrate to winston@3 API Remove some obvious pitfalls. Migrate to winston@3 API Oct 4, 2017
@indexzero
Copy link
Contributor Author

indexzero commented Oct 4, 2017

This is good to merge since winston@3.0.0-rc0 has been released to the world.

@jsumners the default API should considered deprecated as of winston@3.0.0-rc0. Will be making that clear in the README.md of winston and removing it in winston@4.

Therefore, I assume the way Winston gets used the majority of the time is via the simpler method: the default logger. Am I wrong?

Also: yes, this is incorrect. Most issues / samples I've seen have new winston.Logger in them.

@jsumners
Copy link
Member

jsumners commented Oct 4, 2017

Very nice work on speeding up Winston; those are some impressive gains.

I think we should wait to merge until 3.0.0 is out of release candidate stage.

@mcollina
Copy link
Member

mcollina commented Oct 6, 2017

I'm ok with waiting, I think @indexzero has plans to move it to out of rc in the next week or so, I hope we don't wait too long.

@jsumners
Copy link
Member

jsumners commented Apr 6, 2018

What should we do with this PR?

@indexzero
Copy link
Contributor Author

indexzero commented Apr 6, 2018

I would suggest merging it. We're on winston@3.0.0-rc3 now (soon to be 3.0.0-rc4) – we're getting close to a final release, but there have been no material API changes.

(EDIT @indexzero): in rc 3.0.0 has gotten a good percentage of the userbase – the vast majority of new issues are about 3.0.0-rc*.

@jsumners
Copy link
Member

jsumners commented Apr 6, 2018

Okay. Please remember to give us a drive-by commit to bump the version when you go final.

@jsumners jsumners merged commit 3ed4fcd into pinojs:master Apr 6, 2018
@indexzero indexzero deleted the winston3 branch April 6, 2018 13:03
@indexzero
Copy link
Contributor Author

@jsumners will do! Thanks for maintaining these benchmarks as part of pino.

@github-actions
Copy link

github-actions bot commented Feb 7, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants