Skip to content

Comments

Add event loop utilization support#48

Merged
devinivy merged 8 commits intov8from
feature/support-event-loop-utilization
May 8, 2022
Merged

Add event loop utilization support#48
devinivy merged 8 commits intov8from
feature/support-event-loop-utilization

Conversation

@jonathansamines
Copy link
Contributor

Introduces support to track the event loop utilization, the intent is to add this support to hapi itself as described at hapijs/hapi#4288

Notes:

  • performance.eventLoopUtilization is available from Node 12.19.0. Not certain if introducing this support would be a breaking change or not.

Copy link

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

You might want update the load values on a check(), similar to eventLoopDelay when elapsed > this.settings.sampleInterval. This should also fix the tests and not need the excessive delay() values.

Actually, looking at it, I'm wondering why check() doesn't do a full measure when elapsed > this.settings.sampleInterval. The memory values have probably also changed, and might be relevant.

@devinivy
Copy link
Member

devinivy commented Oct 1, 2021

One consideration: this could break for users of node v12 who aren't on node v12.19+. We could add a temporary fallback behavior until we drop node v12 hapijs/hapi#4279, or wait for hapi v21.

@jonathansamines
Copy link
Contributor Author

You might want update the load values on a check(), similar to eventLoopDelay when elapsed > this.settings.sampleInterval. This should also fix the tests and not need the excessive delay() values.

Thanks for the suggestion @kanongil, tests pass consistently now

Actually, looking at it, I'm wondering why check() doesn't do a full measure when elapsed > this.settings.sampleInterval. The memory values have probably also changed, and might be relevant.

As for updating memory values, I think it does make sense. Do you want me to update those as well?

Copy link

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

Found a critical issue.

@jonathansamines
Copy link
Contributor Author

One consideration: this could break for users of node v12 who aren't on node v12.19+. We could add a temporary fallback behavior until we drop node v12 hapijs/hapi#4279, or wait for hapi v21.

Right, I think is fine if we wait for hapi 21 to land this changes. How far in the future would that be?

@devinivy
Copy link
Member

devinivy commented Oct 9, 2021

It's TBD but I am a proponent for making this happen relatively soon in order to leave a gap before the EOL for node v12.

@devinivy devinivy added the feature New functionality or improvement label May 8, 2022
@devinivy devinivy added this to the 8.0.0 milestone May 8, 2022
@devinivy devinivy changed the base branch from master to v8 May 8, 2022 21:36
@devinivy devinivy dismissed kanongil’s stale review May 8, 2022 21:37

Requested changes have been addressed.

@devinivy devinivy merged commit 6f3be46 into v8 May 8, 2022
@devinivy devinivy deleted the feature/support-event-loop-utilization branch May 8, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New functionality or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants