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

Make pino.destination sync #566

Merged
merged 6 commits into from
Dec 11, 2018
Merged

Make pino.destination sync #566

merged 6 commits into from
Dec 11, 2018

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Dec 9, 2018

Fixes #542

@mcollina
Copy link
Member Author

mcollina commented Dec 9, 2018

@yonjah @brandondoran @pies @zyf0330 please test!

@zyf0330
Copy link

zyf0330 commented Dec 10, 2018

With this fix, I don't must need pino.final or pino.destination to output complete log followed by process.exit(), just normal pino logger.

@yonjah
Copy link
Contributor

yonjah commented Dec 10, 2018

I did a few basic tests and it seem to be working with no issues

Copy link
Member

@davidmarkclements davidmarkclements left a comment

Choose a reason for hiding this comment

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

more comments to come in-thread

test/final.test.js Outdated Show resolved Hide resolved
test/destination-exit.test.js Outdated Show resolved Hide resolved
@davidmarkclements
Copy link
Member

davidmarkclements commented Dec 10, 2018

  1. I think this means we can remove (or at least deprecate) destination.flushSync
  2. In the docs we can remove https://github.com/pinojs/pino/blame/master/docs/api.md#L665-L666
  3. need to adjust pino.final docs (starting: https://github.com/pinojs/pino/blame/master/docs/api.md#L703) with an intro saying ONLY FOR pino.extreme and then remove all references to pino.destination

@mcollina
Copy link
Member Author

I think this means we can remove (or at least deprecate) destination.flushSync

destination and extreme are in fact the same thing, configured differently. We need it for extreme, so we get it in destination as well.

In the docs we can remove https://github.com/pinojs/pino/blame/master/docs/api.md#L665-L666

Definitely.

need to adjust pino.final docs (starting: https://github.com/pinojs/pino/blame/master/docs/api.md#L703) with an intro saying ONLY FOR pino.extreme MODE and then remove all references to pino.destination

I'll do that.

@mcollina mcollina dismissed davidmarkclements’s stale review December 11, 2018 10:20

everything has been addressed

@mcollina
Copy link
Member Author

@davidmarkclements @jsumners PTAL, it should be ready to land.

I've removed the lambda hack, as it should not be needed anymore.

@davidmarkclements davidmarkclements merged commit 5233273 into master Dec 11, 2018
@mcollina mcollina deleted the make-destination-sync branch December 14, 2018 08:46
tlvince added a commit to tlvince/pino that referenced this pull request Aug 14, 2019
In pinojs#566, the Lambda hack was removed. Seems this was missed from the
docs. AFAICT, the [Lambda reference][1] in `pino-extreme` is still
relevant.

[1]: https://github.com/pinojs/pino/blob/43d398800fc8b0de189ccd9b11bc2afdb28524d9/docs/api.md#pinoextremetarget--sonicboom
mcollina pushed a commit that referenced this pull request Aug 15, 2019
In #566, the Lambda hack was removed. Seems this was missed from the
docs. AFAICT, the [Lambda reference][1] in `pino-extreme` is still
relevant.

[1]: https://github.com/pinojs/pino/blob/43d398800fc8b0de189ccd9b11bc2afdb28524d9/docs/api.md#pinoextremetarget--sonicboom
@github-actions
Copy link

github-actions bot commented Feb 5, 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 5, 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.

Pino 5.8.1 fails to flush log on process exit
4 participants