pino.transport() #1003

mcollina · 2021-04-07T10:10:25Z

This add pino.transport() based on thread-stream.

In theory, this supersedes #937 as we internalize the pinole part.

Supports Node v12.20.0 onwards.

Things that are missing:

Docs
transport example/boilerplate
Support for transport packages
~~Support for pure function transports (is this actually needed?)~~
More tests
pino-specific benchmarks
test support for transport packages on windows

lib/transport.js

jsumners · 2021-04-07T12:02:13Z

Support for pure function transports (is this actually needed?)

I think so.

This reverts commit b3e4178.

lib/transport.js

Co-authored-by: James Sumners <[email protected]>

This reverts commit ec34df7.

This reverts commit 246b6f0.

mcollina · 2021-04-07T16:34:22Z

Support for pure function transports (is this actually needed?)

I think so.

Why is that so? I'm not really understanding the need of it.
Processing one log line at a time is far from fast and it will generate a significant build-up in memory.

jsumners · 2021-04-07T16:56:59Z

Processing one log line at a time is far from fast

I suppose I didn't quite follow what was meant by the item.

mcollina · 2021-04-07T17:10:19Z

pinole expects a transport to be a pure function: https://github.com/pinojs/pinole/blob/f347c04d4a6c61e05e6ac821e510548aab928458/lib/worker.mjs#L18-L28.
Then it uses await to process every incoming chunk of data one at a time.

You can also see this at https://github.com/pinojs/pino/pull/937/files#diff-b230460830160b31f0c5495a4524feaf9cbc4adba704dbeae6ef3485c45d5b5d

package.json

lib/transport.js

package.json

test/fixtures/transport/index.js

test/transport.test.js

davidmarkclements · 2021-06-11T16:40:27Z

test/transport.test.js

+  )
+  const transport = pino.transport({
+    targets: [{
+      level: 'info',


how does this work with custom levels?

can the level option be supported as a top level option as well?

how does this work with custom levels?

I literally have no clue, I'm not convinced it would work as it is. I would recommend to document that it does not work with custom levels right now.

Co-authored-by: David Mark Clements <[email protected]>

mcollina · 2021-07-02T22:30:10Z

@jsumners I think this is ready to go. Could you take a look so we can ship it?

kibertoad · 2021-07-02T23:06:38Z

@mcollina please ping me to revise types after this lands

docs/api.md

docs/transports.md

jsumners · 2021-07-03T03:31:40Z

docs/api.md

@@ -905,6 +906,98 @@ A `pino.destination` instance can also be used to reopen closed files
 * See [Reopening log files](/docs/help.md#reopening)
 * See [Asynchronous Logging ⇗](/docs/asynchronous.md)

+<a id="pino-transport"></a>
+### `pino.transport(options) => ThreadStream`


There should be a callout here, and in the transports specific docs, to notify the reader about CPU core count. Ideally, a core for the main app + a core per worker thread (transport) should be available.

jsumners · 2021-07-03T03:32:12Z

lib/file-target.js

+const { once } = require('events')
+
+module.exports = async function (opts) {
+  // TODO remove the transform


jsumners · 2021-07-03T03:32:18Z

lib/file-target.js

+    }
+  })
+
+  // TODO figure out why sync: false does not work here


jsumners · 2021-07-03T03:32:37Z

lib/pretty-target.js

+  // TODO figure out why sync: false does not work here
+  // TODO remove the istanbul ignore, add a test using a
+  // child_process


jsumners · 2021-07-03T03:33:09Z

lib/transport.js

+
+    if (workerOpts.autoEnd !== false) {
+      // TODO possibly use FinalizationGroup to automatically remove


I prefer to not resolve this here. There is a space for a tiny little utility for doing exactly this, however it's better developed in isolation. It's more a note for improvements in the future.

This means we are leaking memory if a transport is not closed. I think it's ok.

Co-authored-by: James Sumners <[email protected]>

mcollina · 2021-07-03T08:38:47Z

I've addressed most comments @jsumners

mcollina · 2021-07-03T13:48:34Z

@kibertoad could you please send a PR to update the types?

kibertoad · 2021-07-03T13:49:04Z

@mcollina I take it api is final and I can add types?

kibertoad · 2021-07-03T13:49:15Z

on it!

mcollina · 2021-07-03T13:49:59Z

Yes it should be. It might change further in the -rc cycle once we gather feedback, but we are at a point where it's better to give this in the hands of users.

kibertoad · 2021-07-03T18:26:56Z

@mcollina Are you sure that multiple transport example in docs/api.md is correct?

When I'm trying this:

const transports = pino.transport([
    {
        level: 'info',
        target: '#pino/pretty',
        options: { some: 'options for', the: 'transport' }
    },
    {
        level: 'trace',
        target: '#pino/file',
        options: { destination: './' }
    }
])

It explodes on if (isAbsolute(origin) || origin.indexOf('file://') === 0) {, because target is not set.

mcollina · 2021-07-04T09:53:33Z

good spot: #1049

github-actions · 2022-11-21T00:05:04Z

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.

Basic transport implemented on top of ThreadStream

a2840c8

mcollina requested a review from davidmarkclements April 7, 2021 10:11

mcollina added 2 commits April 7, 2021 12:29

Support module transports

b2404c9

Skip package test on win32

6b7d7bb

jsumners reviewed Apr 7, 2021

View reviewed changes

lib/transport.js Show resolved Hide resolved

lib/transport.js Show resolved Hide resolved

mcollina added 4 commits April 7, 2021 14:51

Ire-enabled failing test on windows

b3b537a

full cov report

b3e4178

Revert "full cov report"

ec34df7

This reverts commit b3e4178.

ignore lines unreacheable on Windows

ed8cfd5

jsumners reviewed Apr 7, 2021

View reviewed changes

lib/transport.js Outdated Show resolved Hide resolved

lib/transport.js Outdated Show resolved Hide resolved

jsumners reviewed Apr 7, 2021

View reviewed changes

lib/transport.js Outdated Show resolved Hide resolved

lib/transport.js Outdated Show resolved Hide resolved

mcollina and others added 5 commits April 7, 2021 18:04

Update lib/transport.js

e1c72b9

Co-authored-by: James Sumners <[email protected]>

Update lib/transport.js

44c150c

Co-authored-by: James Sumners <[email protected]>

Revert "Revert "full cov report""

246b6f0

This reverts commit ec34df7.

try out different ignore strategies

eeca496

Revert "Revert "Revert "full cov report"""

661de3e

This reverts commit 246b6f0.

mcollina mentioned this pull request Apr 7, 2021

Add pino.multistream #1004

Merged

mcollina added the v7 label Apr 7, 2021

Works on Windows

d78a30b

jsumners reviewed Apr 8, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

Emit 'error' if thread exits abruptly

c54769e

davidmarkclements reviewed Apr 8, 2021

View reviewed changes

lib/transport.js Outdated Show resolved Hide resolved

davidmarkclements reviewed Apr 8, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

davidmarkclements reviewed Apr 8, 2021

View reviewed changes

test/fixtures/transport/index.js Show resolved Hide resolved

davidmarkclements reviewed Apr 8, 2021

View reviewed changes

test/fixtures/transport/index.js Outdated Show resolved Hide resolved

davidmarkclements mentioned this pull request Apr 8, 2021

[ WIP ] pinole integration, next gen transports #937

Closed

davidmarkclements reviewed Jun 11, 2021

View reviewed changes

test/transport.test.js Outdated Show resolved Hide resolved

davidmarkclements reviewed Jun 11, 2021

View reviewed changes

test/transport.test.js Outdated Show resolved Hide resolved

davidmarkclements reviewed Jun 11, 2021

View reviewed changes

davidmarkclements and others added 3 commits June 11, 2021 18:56

built in docs, other doc tweaks

82ebbbf

Update test/transport.test.js

2474e29

Co-authored-by: David Mark Clements <[email protected]>

Update test/transport.test.js

6c49f3e

Co-authored-by: David Mark Clements <[email protected]>

miladezzat approved these changes Jun 20, 2021

View reviewed changes

mcollina marked this pull request as ready for review July 2, 2021 22:29

jsumners reviewed Jul 3, 2021

View reviewed changes

mcollina and others added 6 commits July 3, 2021 10:06

Update docs/api.md

17dc8a9

Co-authored-by: James Sumners <[email protected]>

Update docs/api.md

8ef4409

Co-authored-by: James Sumners <[email protected]>

Update docs/api.md

8855a33

Co-authored-by: James Sumners <[email protected]>

Update docs/transports.md

224474e

Co-authored-by: James Sumners <[email protected]>

Update docs/transports.md

55e8b61

Co-authored-by: James Sumners <[email protected]>

PR reviews

45cb21c

jsumners approved these changes Jul 3, 2021

View reviewed changes

mcollina merged commit 849f65a into next Jul 3, 2021

kibertoad mentioned this pull request Jul 4, 2021

Add TS types for transports #1050

Merged

jsumners deleted the transport branch November 20, 2021 18:04

github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pino.transport() #1003

pino.transport() #1003

mcollina commented Apr 7, 2021 •

edited by davidmarkclements

Loading

jsumners commented Apr 7, 2021

mcollina commented Apr 7, 2021

jsumners commented Apr 7, 2021

mcollina commented Apr 7, 2021 •

edited

Loading

davidmarkclements Jun 11, 2021

davidmarkclements Jun 11, 2021

mcollina Jun 12, 2021

mcollina commented Jul 2, 2021

kibertoad commented Jul 2, 2021

jsumners Jul 3, 2021

jsumners Jul 3, 2021

jsumners Jul 3, 2021

jsumners Jul 3, 2021

jsumners Jul 3, 2021

mcollina Jul 3, 2021

mcollina commented Jul 3, 2021

mcollina commented Jul 3, 2021

kibertoad commented Jul 3, 2021

kibertoad commented Jul 3, 2021

mcollina commented Jul 3, 2021

kibertoad commented Jul 3, 2021 •

edited

Loading

mcollina commented Jul 4, 2021

github-actions bot commented Nov 21, 2022


		if (workerOpts.autoEnd !== false) {
		// TODO possibly use FinalizationGroup to automatically remove

pino.transport() #1003

pino.transport() #1003

Conversation

mcollina commented Apr 7, 2021 • edited by davidmarkclements Loading

jsumners commented Apr 7, 2021

mcollina commented Apr 7, 2021

jsumners commented Apr 7, 2021

mcollina commented Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Jul 2, 2021

kibertoad commented Jul 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Jul 3, 2021

mcollina commented Jul 3, 2021

kibertoad commented Jul 3, 2021

kibertoad commented Jul 3, 2021

mcollina commented Jul 3, 2021

kibertoad commented Jul 3, 2021 • edited Loading

mcollina commented Jul 4, 2021

github-actions bot commented Nov 21, 2022

mcollina commented Apr 7, 2021 •

edited by davidmarkclements

Loading

mcollina commented Apr 7, 2021 •

edited

Loading

kibertoad commented Jul 3, 2021 •

edited

Loading