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

Reduce overhead of monitor pipeline #5

Merged
merged 3 commits into from
Dec 2, 2020
Merged

Conversation

watson
Copy link

@watson watson commented Nov 30, 2020

This PR optimizes two things:

Use pipeline over pumpify

This is mainly to fix an "issue" with Node.js 14 where pumpify requires a few extra ticks every time someone writes to the pipeline. This could result in the process crashing prior to the logger writing to its destination. For example, in the following example, the data event-listener would never be called before the process crashes because of the thrown error on Node.js 14:

const Pumpify = require('pumpify')
const { PassThrough } = require('stream')
const a = new PassThrough()
const b = new PassThrough()
const pipe = Pumpify([a, b])
b.on('data', (chunk) => console.log('Got data:', chunk.toString()))
pipe.write('hello')
throw new Error('boom!')

However, the following would call the data event-listener on Node.js 14:

const { PassThrough, pipeline } = require('stream')
const a = new PassThrough()
const b = new PassThrough()
pipeline(a, b, () => {})
b.on('data', (chunk) => console.log('Got data:', chunk.toString()))
a.write('hello')
throw new Error('boom!')

Don't use pipeline if not required

In the case where there's only one stream, there's no need to use a pipeline. Instead we can just write directly to the stream. This also reduces the overhead of the logger slightly.

Copy link

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

const stream = streams[0];

if (streams.length === 1) {
finished(stream, onFinished);
Copy link

Choose a reason for hiding this comment

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

Nice, didn't know about the finished() helper!

Copy link

Choose a reason for hiding this comment

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

OMG, finally it's provided out-of-the-box

Copy link

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Oops, missed the test failure

@watson watson requested a review from spalger December 1, 2020 08:10
Copy link

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

Considering that @hapi/good is EOL on 31.12.2020, I'm okay with this divergence from the upstream. I created elastic/kibana#82240 to remove the dependency on @hapi/good package.

@watson watson changed the title Reduse overhead of monitor pipeline Reduce overhead of monitor pipeline Dec 1, 2020
Copy link

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@watson
Copy link
Author

watson commented Dec 1, 2020

@spalger my latest commit only fixed part of the CI issue. There's still some failures. However, it looks like master is also failing currently. That being said, it also looks like the failures in this PR is slightly different from the ones in master, so I'll take a look tomorrow to see what's going on.

@watson
Copy link
Author

watson commented Dec 2, 2020

Sorry for messing up the PR with a lot of unrelated commits. I just rebased this PR on top of #6 to get a more stable CI. Once #6 is merged into master, I'll rebase this PR with master to clean it up.

Copy link

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for fixing CI!

@watson watson merged commit 1bf8478 into elastic:master Dec 2, 2020
@watson watson deleted the fix-pipeline branch December 2, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants